From fe450bdbe5dcaf9c63ba65827ab618a3fa906e29 Mon Sep 17 00:00:00 2001 From: ShotaroKataoka Date: Sat, 1 Aug 2026 12:10:26 +0900 Subject: [PATCH 1/5] =?UTF-8?q?=E2=9C=A8=20feat(run-python-unified-semanti?= =?UTF-8?q?cs):=20writes=20always=20persist;=20deprecate=20save=20flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local and Remote run_python had divergent 'save' semantics: Local persisted writes unconditionally (save only gated the build), Remote silently discarded writes without save=True. The v0.5.1 persona unification exposed this as agent instability in the cloud E2E (vibe agent 'forgetting' save=True). New unified contract: - File writes always persist — no 'unsaved' state, no flag to forget - PPTX artifact rebuilds automatically when the deck changed (cheap build) - measure_slides is the only trigger for the expensive verification pass - save is accepted but ignored (deprecation note in result) - Remote write-back is diff-based (baseline snapshot), fixing the stale-copy clobber risk; read-only users run without persistence (readOnly note) - run_style_python (Remote) unified the same way: style.html persists automatically when changed personas/guides/wiring/docs updated (repo-wide save= mentions: 0). tests/test_run_python_semantics.py pins the semantics on both adapters. SPEC: 20260801-1122_run-python-unified-semantics Progress: Phase 0-2 complete; make all 628 passed / lint clean Next: redeploy cloud stack, user re-runs compose E2E (v0.5.1 gate) --- CHANGELOG.md | 11 ++ agent/prompts/wiring/style_remote.md | 6 +- clients/claude-code/agents/sdpm-composer.md | 2 +- docs/en/architecture.md | 4 +- personas/composer.md | 23 ++- personas/single.md | 2 +- personas/spec.md | 2 +- personas/vibe.md | 6 +- sdpm/references/guides/import-pptx.md | 31 ++- servers/local/sandbox_tools.py | 58 ++++-- servers/remote/server.py | 125 +++++++++--- servers/remote/tools/sandbox.py | 70 ++++--- tests/test_agent_modes.py | 9 +- tests/test_run_python_semantics.py | 208 ++++++++++++++++++++ 14 files changed, 454 insertions(+), 103 deletions(-) create mode 100644 tests/test_run_python_semantics.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ddaa933..502f3d61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,17 @@ Entries before v0.5.0 were written retroactively as summaries. ## [Unreleased] +### Changed + +- `run_python` / `run_style_python`: file writes now always persist — the + `save` flag is deprecated and ignored (silent data loss when omitting + `save=True` on Cloud is no longer possible). The deck's PPTX artifact + refreshes automatically whenever the deck changes; `measure_slides` + remains the trigger for the expensive verification pass (render, text + overflow measurement, previews) +- Cloud sandbox write-back is now diff-based (changed/new files only), + preventing a stale sandbox copy from overwriting newer S3 writes + ## [0.5.1] - 2026-07-31 ### Added diff --git a/agent/prompts/wiring/style_remote.md b/agent/prompts/wiring/style_remote.md index ba7e6db0..f98b07fb 100644 --- a/agent/prompts/wiring/style_remote.md +++ b/agent/prompts/wiring/style_remote.md @@ -4,14 +4,14 @@ In this environment, `run_style_python` does NOT inject `read_style` / `write_style` helpers. Use normal file I/O with these parameters instead: Workspace layout: -- `style.html` — the target style file (read/write; saved back when `save=True`) +- `style.html` — the target style file (read/write; persisted automatically when changed) - `ref/{name}.html` — reference styles (read-only; loaded via the `ref_styles` parameter) Usage patterns: - Read a reference: `run_style_python(code="html = open('ref/corporate-executive.html').read(); print(html[:500])", ref_styles=["corporate-executive"])` -- Create/edit the style: `run_style_python(code="open('style.html','w').write(html)", style_name="style-20260506-1430", save=True)` +- Create/edit the style: `run_style_python(code="open('style.html','w').write(html)", style_name="style-20260506-1430")` - Read back for incremental edits: `run_style_python(code="print(open('style.html').read())", style_name="style-20260506-1430")` The user's first message contains `[Style: ]` — pass it as the `style_name` parameter on every call (not as a `write_style` argument). -`save=True` is what persists the style; without it your edits are discarded. +Writes to style.html persist automatically — there is no save flag. diff --git a/clients/claude-code/agents/sdpm-composer.md b/clients/claude-code/agents/sdpm-composer.md index 97e47bab..7f298a68 100644 --- a/clients/claude-code/agents/sdpm-composer.md +++ b/clients/claude-code/agents/sdpm-composer.md @@ -18,5 +18,5 @@ your **assigned slide slugs**. You write ONLY those slugs. Work silently — no interaction, no Phase 3. Client note: you may open preview PNG files with the CC-native **Read** tool, but write -deck files only through `run_python` (never Write/Edit — `save=True` must stay the +deck files only through `run_python` (never Write/Edit — `run_python` must stay the single writer). diff --git a/docs/en/architecture.md b/docs/en/architecture.md index 68322934..8061cf7a 100644 --- a/docs/en/architecture.md +++ b/docs/en/architecture.md @@ -96,8 +96,8 @@ S3 (resource bucket): ### Deck Workspace -Using `run_python(deck_id=..., save=True)` loads the entire deck workspace into the sandbox. -The agent can read and write files using standard Python file I/O (`open`, `json.load`, etc.), and `save=True` writes changes back to S3. +Using `run_python(deck_id=...)` loads the entire deck workspace into the sandbox. +The agent can read and write files using standard Python file I/O (`open`, `json.load`, etc.); modified files are written back to S3 automatically after every execution. ``` deck.json — deck metadata (template, fonts, defaultTextColor) diff --git a/personas/composer.md b/personas/composer.md index 71b7b871..a4087596 100644 --- a/personas/composer.md +++ b/personas/composer.md @@ -61,7 +61,7 @@ are written for the CLI (`pptx_builder.py …`); on MCP translate every CLI comm | `pptx_builder.py workflows ` | `read_workflows([""])` | | `pptx_builder.py guides ` | `read_guides([""])` | | `pptx_builder.py examples ` | `read_examples([""])` | -| `pptx_builder.py measure {json} -p {n}` | `run_python(..., save=True, measure_slides=["{slug}"])` | +| `pptx_builder.py measure {json} -p {n}` | `run_python(..., measure_slides=["{slug}"])` | | `pptx_builder.py preview {json}` | covered by the same call — it returns `preview_files` | | `pptx_builder.py image-size {path} --width {px}` | no tool — compute proportional size in `run_python` (`new_h = round(orig_h * target_w / orig_w)`) | | `pptx_builder.py code-block …` | `code_to_slide(...)` | @@ -82,24 +82,23 @@ data = { write_json("slides/{slug}.json", data) ''', deck_id="", - save=True, measure_slides=["{slug}"], ) ``` -`save=True` triggers `lint_and_sanitize` (it rewrites `slides/{slug}.json`), the PPTX -build, PNG render, and returns `preview_files` (PNG paths), `warnings`, and -`lint_diagnostics` — filtered to the slugs you measured. Do not write deck files through -any other mechanism (no client-native Write/Edit) — `save=True` must stay the single -writer. Inside the sandbox use `read_json` / `read_text` / `list_files`; `open()` is -blocked. +Writes always persist — there is no save flag. `measure_slides` triggers +`lint_and_sanitize` (it rewrites `slides/{slug}.json`), the PPTX build, PNG render, and +returns `preview_files` (PNG paths), `warnings`, and `lint_diagnostics` — filtered to +the slugs you measured. Do not write deck files through any other mechanism (no +client-native Write/Edit) — `run_python` must stay the single writer. Inside the +sandbox use `read_json` / `read_text` / `list_files`; `open()` is blocked. -### Per-slide save loop (MANDATORY) +### Per-slide write loop (MANDATORY) -Write and save **one slide at a time** — never batch-write multiple `slides/*.json` in a +Write **one slide at a time** — never batch-write multiple `slides/*.json` in a single call (risks output truncation). Per slug: -**write → `run_python(save=True, measure_slides=["{slug}"])` → inspect returned +**write → `run_python(measure_slides=["{slug}"])` → inspect returned `preview_files` + `warnings` → fix if needed → next slug.** ## Working Philosophy @@ -159,7 +158,7 @@ own **every** slide in the deck for this call. Read all `slides/*.json` directly overlap, alignment on a single slide) are OUT OF SCOPE — do not touch them, even if you notice them; a separate per-slide fix pass handles those. -Fix via `run_python(save=True, measure_slides=[...])`. If the deck is already +Fix via `run_python(measure_slides=[...])`. If the deck is already consistent, respond with a brief summary and return — over-editing causes new inconsistencies. diff --git a/personas/single.md b/personas/single.md index 0f64e253..382be490 100644 --- a/personas/single.md +++ b/personas/single.md @@ -5,7 +5,7 @@ Respond in the same language as the user. spec-driven-presentation-maker: AI-powered PowerPoint generation from JSON. ## Architecture -- The agent edits workspace files via `run_python(deck_id=..., save=True)` using normal file I/O +- The agent edits workspace files via `run_python(deck_id=...)` using normal file I/O (writes always persist) - MCP tools handle: workflow guidance, initialization, PPTX generation, preview, references - MCP tools do NOT handle: slide editing, spec writing (agent responsibility via run_python) diff --git a/personas/spec.md b/personas/spec.md index 7bb1304c..6317c3c2 100644 --- a/personas/spec.md +++ b/personas/spec.md @@ -172,7 +172,7 @@ Each group runs as an independent composer in parallel. Groups cannot share info inconsistencies (labeling, decorative elements, typography, writing style, hierarchy). 2. **Verification**: view the post-review renders yourself. If you composed sequentially, use the `preview_files` returned from your - `run_python(save=True)` calls. If you dispatched composers, their tool + `run_python(measure_slides=[...])` calls. If you dispatched composers, their tool results are not visible to you — view the previews another way (this is the one exception to "do not call preview tools directly"): - a `get_preview` tool exists → call `get_preview(deck_id, slugs=[...all slugs...])` diff --git a/personas/vibe.md b/personas/vibe.md index bd2a8c06..256eec43 100644 --- a/personas/vibe.md +++ b/personas/vibe.md @@ -134,7 +134,7 @@ First load references (read_workflows(["create-new-2-compose","slide-json-spec"] read_guides(["grid"]), read_examples(["components/all","patterns"])), then read specs/brief.md, specs/outline.md, specs/art-direction.html for context. Compose ONLY your assigned slugs, one at a time, via run_python's write_json, and use -the preview_files (PNG) returned by run_python(save=True, measure_slides=[slug]) as the +the preview_files (PNG) returned by run_python(measure_slides=[slug]) as the source of truth. Do NOT touch other slides, deck.json, or specs/. art-direction is FROZEN. Do NOT advance to Phase 3. Return a summary plus any warnings. ``` @@ -163,8 +163,8 @@ Each group runs as an independent composer in parallel. Groups cannot share info 1. **Consistency review pass**: dispatch a single composer with ALL slugs in the deck and the instruction: "Consistency review." 2. **Verification**: view the post-review renders yourself. If you composed - sequentially, use the `preview_files` returned from your `run_python(save=True)` - calls. If you dispatched composers, their tool results are not visible to you — + sequentially, use the `preview_files` returned from your + `run_python(measure_slides=[...])` calls. If you dispatched composers, their tool results are not visible to you — view the previews another way (the one exception to "do not call preview tools directly"): call `get_preview(deck_id, slugs=[...all slugs...])` if that tool exists, otherwise read the PNG files at `/preview/.png` with your diff --git a/sdpm/references/guides/import-pptx.md b/sdpm/references/guides/import-pptx.md index c3e1f1e0..ce7762e0 100644 --- a/sdpm/references/guides/import-pptx.md +++ b/sdpm/references/guides/import-pptx.md @@ -95,9 +95,9 @@ Populate `specs/brief.md` and `specs/outline.md` **before** Step 4 builds the deck. `specs/art-direction.html` is intentionally deferred to Step 5 — the rendered slide previews from Step 4 are a far better input for it than the upload-time image extraction. Each sub-step -uses `run_python(save=True)` so the intermediate state is persisted — -Cloud discards the sandbox VM between calls, so `save=False` would -lose the write. +uses `run_python` — writes always persist automatically (no save +flag), so intermediate state survives even though Cloud discards the +sandbox VM between calls. You generate these specs from the PPTX content you imported in Step 2. Do not call `hearing` in Step 3 — if a particular field is thin, leave @@ -123,10 +123,10 @@ for name in sorted(files): print(name, "::", title) ``` -Run that via `run_python(code=, deck_id=deck_id, save=False)` +Run that via `run_python(code=, deck_id=deck_id)` (Cloud: prepend `purpose="Inspect PPTX slides"`). -Then write `specs/brief.md` in a second call with `save=True`: +Then write `specs/brief.md` in a second call: ```python short_id = "" @@ -146,7 +146,7 @@ write_text("specs/brief.md", "\n".join(lines) + "\n") print("brief.md written") ``` -Call as `run_python(code=, deck_id=deck_id, save=True)` +Call as `run_python(code=, deck_id=deck_id)` (Cloud: prepend `purpose="Write brief.md from PPTX content"`). ### 3-2. outline.md (LLM summarization) @@ -167,7 +167,7 @@ write_text("specs/outline.md", "\n".join(lines) + "\n") print("outline.md written:", len(pairs)) ``` -Call with `run_python(code=, deck_id=deck_id, save=True)` +Call with `run_python(code=, deck_id=deck_id)` (Cloud: add `purpose="Write outline.md from PPTX content"`). Requirements (outline lint will otherwise reject the write on Cloud): @@ -183,12 +183,12 @@ Requirements (outline lint will otherwise reject the write on Cloud): Copy the PPTX-derived slide JSON into `slides/`, merge deck metadata into `deck.json` (using the deck-local `template.pptx`), and build the -deck in a **single** `run_python` call with `save=True`. +deck in a **single** `run_python` call with `measure_slides`. **Do not split Step 4 into multiple calls.** Each Cloud `run_python` -runs in a fresh sandbox VM that is discarded afterward, so intermediate -`save=False` writes are lost. Keeping Step 4 in one call ensures the -copy, S3 writeback, build, preview, and compose all share a single VM. +runs in a fresh sandbox VM that is discarded afterward. Keeping Step 4 +in one call ensures the copy, S3 writeback, build, preview, and compose +all share a single VM. Assemble the slug list from Step 3-2 as a Python literal: @@ -261,16 +261,15 @@ Call as: run_python( code=, deck_id=deck_id, - save=True, measure_slides=slugs, ) ``` Cloud: prepend `purpose="Import PPTX slides into deck and build"`. -Because `specs/outline.md` was populated in Step 3-2, `save=True` -triggers a full build that includes every slide, followed by preview -and SVG compose. The PPTX-derived placeholder template means **layout +Because `specs/outline.md` was populated in Step 3-2, the build +includes every slide, followed by preview and SVG compose (triggered +by `measure_slides`). The PPTX-derived placeholder template means **layout mismatch is impossible** — the build should succeed in one shot. After the `run_python` call returns successfully, call @@ -510,7 +509,7 @@ lists, charts, or specific data into the demonstration slides — that content lives in `slides/` (placed by Step 4), not in the style specification. -Write incrementally via `run_python(save=True)` — one call for the +Write incrementally via `run_python` — one call for the skeleton + `:root` + first slide, then one or two more for the remaining slides (per the create-style workflow's incremental writing guidance): diff --git a/servers/local/sandbox_tools.py b/servers/local/sandbox_tools.py index 5fe72044..ece3550f 100644 --- a/servers/local/sandbox_tools.py +++ b/servers/local/sandbox_tools.py @@ -36,6 +36,28 @@ def _rejection_message(violations: list[str], has_deck: bool) -> str: return "\n".join(lines) +def _build_snapshot(deck_dir: Path) -> dict[str, tuple[int, int]]: + """Snapshot (mtime_ns, size) of files that affect the built PPTX. + + Used to detect whether a run_python execution changed the deck, so the + output.pptx artifact can be rebuilt automatically. specs/outline.md is + included because slide order comes from the outline. + """ + snap: dict[str, tuple[int, int]] = {} + for rel in ("deck.json", "presentation.json", "specs/outline.md"): + p = deck_dir / rel + if p.is_file(): + st = p.stat() + snap[rel] = (st.st_mtime_ns, st.st_size) + for sub in ("slides", "includes"): + d = deck_dir / sub + if d.is_dir(): + for p in d.glob("*.json"): + st = p.stat() + snap[f"{sub}/{p.name}"] = (st.st_mtime_ns, st.st_size) + return snap + + def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, measure_slides: list[str] | None = None) -> str: """Execute Python code in a sandboxed environment. @@ -97,11 +119,21 @@ def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, **Always specify measure_slides when editing slides.** + ## Persistence & build (no flags needed) + + - File writes always persist — anything written via write_json/write_text + is saved immediately. There is no "unsaved" state. + - output.pptx rebuilds automatically whenever the deck changed + (deck.json / slides/ / includes/ / specs/outline.md). + - measure_slides triggers the expensive verification pass (render + text + overflow measurement + preview PNGs) for the given slugs only. + Args: purpose: Brief user-facing description of what this code does. Shown in UI. code: Python code to execute (no import statements allowed). deck_id: Deck output_dir path. Optional. - save: When True, triggers PPTX build + preview + SVG compose after execution. + save: Deprecated and ignored. Writes always persist and output.pptx + rebuilds automatically when the deck changed. measure_slides: Slide slugs to measure after execution (e.g. ["title", "feature-a"]). Returns: @@ -109,6 +141,11 @@ def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, """ result: dict[str, Any] = {} cwd = deck_id if deck_id and Path(deck_id).is_dir() else None + if save: + result["deprecated"] = ( + "'save' is ignored: writes always persist and output.pptx " + "rebuilds automatically when the deck changed." + ) from sandbox import check_code, make_runner @@ -117,6 +154,8 @@ def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, result["output"] = _rejection_message(violations, has_deck=bool(cwd)) return json.dumps(result, ensure_ascii=False) + pre_snap = _build_snapshot(Path(cwd)) if cwd else {} + try: runner = make_runner(deck_id if cwd else "") args = [sys.executable, "-c", runner] @@ -176,8 +215,11 @@ def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, errs = result.setdefault("errors", {}) errs["lintDiagnostics"] = lint_diagnostics - # Post-processing: build PPTX + iso measure/compose/preview (lockless) - if save: + # Post-processing: build PPTX + iso measure/compose/preview (lockless). + # output.pptx is a derived artifact — it follows deck changes automatically. + # measure_slides additionally triggers the expensive verification pass. + deck_changed = _build_snapshot(deck_dir) != pre_snap + if deck_changed or measure_slides: import shutil from sdpm.api import generate, parse_outline_slugs @@ -417,19 +459,13 @@ def _fp(c: dict) -> str: shutil.rmtree(iso_dir, ignore_errors=True) else: - # save=True without measure_slides: output.pptx only, skip compose/measure/preview + # Deck changed without measure_slides: output.pptx only, + # skip compose/measure/preview if _build_warnings: result["warnings"] = _build_warnings if _build_lint: result["lint_diagnostics"] = _build_lint - elif measure_slides: - try: - from sdpm.api import measure as _sdpm_measure - result["measure"] = _sdpm_measure(json_path=deck_input, slides=list(measure_slides)) - except Exception as e: - result["measure"] = f"Measure error: {e}" - return json.dumps(result, ensure_ascii=False) diff --git a/servers/remote/server.py b/servers/remote/server.py index 8760104a..01af4397 100644 --- a/servers/remote/server.py +++ b/servers/remote/server.py @@ -55,7 +55,7 @@ _INSTRUCTIONS = """spec-driven-presentation-maker: AI-powered PowerPoint generation from JSON. ## Architecture -- The agent edits workspace files via `run_python(deck_id=..., save=True)` using normal file I/O +- The agent edits workspace files via `run_python(deck_id=...)` using normal file I/O (writes always persist) - MCP tools handle: workflow guidance, initialization, PPTX generation, preview, references - MCP tools do NOT handle: slide editing, spec writing (agent responsibility via run_python) @@ -625,7 +625,16 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = Raw `open()` / `json.load` still work on Cloud for backward compat, but new code should prefer the helpers. - If save=True, all modified/new workspace files are written back to S3. + ## Persistence & build (no flags needed) + + - File writes always persist — modified/new workspace files are written + back to S3 after every execution. There is no "unsaved" state. + (If you only have read access to the deck, writes are discarded and + the result notes it.) + - The deck's PPTX artifact refreshes automatically whenever the deck + changed (deck.json / slides/ / includes/ / specs/outline.md). + - measure_slides triggers the expensive verification pass (render + text + overflow measurement + live-preview compose) for the given slugs only. **Always specify measure_slides when editing slides.** Runs validation after code execution (requires deck_id): @@ -643,11 +652,11 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = data = read_json("slides/title.json") data["elements"][0]["text"] = "New Title" write_json("slides/title.json", data) - # run_python(code=, deck_id="abc", save=True, measure_slides=["title"]) + # run_python(code=, deck_id="abc", measure_slides=["title"]) Edit spec: write_text("specs/brief.md", "# Brief\\n\\nContents...") - # run_python(code=, deck_id="abc", save=True) + # run_python(code=, deck_id="abc") Read deck metadata: deck = read_json("deck.json") @@ -662,7 +671,8 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = Args: code: Python code to execute. deck_id: Deck ID to load workspace from. Optional. - save: If True, save modified workspace files back to S3. Requires deck_id. + save: Deprecated and ignored. Writes always persist and the PPTX + artifact refreshes automatically when the deck changed. files: S3 keys of files to make available in the sandbox. Optional. measure_slides: List of slide slugs to measure after execution. Requires deck_id. purpose: Brief user-facing description of what this code does, @@ -674,19 +684,38 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = """ if measure_slides and not deck_id: return json.dumps({"error": "measure_slides requires deck_id"}) + + result: dict = {} + if save: + result["deprecated"] = ( + "'save' is ignored: writes always persist and the PPTX artifact " + "refreshes automatically when the deck changed." + ) + + # Writes persist by default. If the user only has read access, run the + # sandbox without write-back instead of failing (read-only analysis). + persist_writes = True if deck_id: - _check_deck_access(deck_id, action="edit_slide" if save else "read") + try: + _check_deck_access(deck_id, action="edit_slide") + except ValueError: + _check_deck_access(deck_id, action="read") + persist_writes = False + result["readOnly"] = ( + "You have read-only access to this deck: file writes were " + "not persisted." + ) - output, outline_warnings, lint_diagnostics = sandbox_mod.execute_in_sandbox( + output, outline_warnings, lint_diagnostics, changed_paths = sandbox_mod.execute_in_sandbox( code=code, storage=_storage, region=_region, deck_id=deck_id, - save=save, + persist_writes=persist_writes, files=files, ) - result: dict = {"output": output} + result["output"] = output if outline_warnings: result.setdefault("warnings", {})["outline"] = ( @@ -698,8 +727,17 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = errs = result.setdefault("errors", {}) errs["lintDiagnostics"] = lint_diagnostics - # Post-processing: measure_slides triggers PPTX build → measure/bias - if deck_id and (measure_slides or save): + # Post-processing: rebuild the PPTX artifact whenever build-relevant files + # changed (the artifact follows the deck automatically); measure_slides + # additionally triggers the verification pass. + def _build_relevant(p: str) -> bool: + return ( + p in ("deck.json", "presentation.json", "specs/outline.md") + or p.startswith(("slides/", "includes/")) + ) + + deck_changed = any(_build_relevant(p) for p in changed_paths) + if deck_id and (measure_slides or deck_changed): import shutil import traceback @@ -753,7 +791,35 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = "available": e["available"], } - if save: + if deck_changed: + # Refresh the download artifact — the deck's PPTX follows deck + # changes automatically (same upload/record shape as + # generate_pptx; WebP previews and KB sync stay with + # generate_pptx, the explicit finalize/handoff step). + try: + import uuid as _uuid + from datetime import datetime as _dt, timezone as _tz + _pptx_key = f"pptx/{deck_id}/{_uuid.uuid4()}.pptx" + _storage.upload_file( + key=_pptx_key, + data=Path(pptx_path).read_bytes(), + content_type=( + "application/vnd.openxmlformats-officedocument" + ".presentationml.presentation" + ), + ) + _storage.update_deck( + deck_id=deck_id, user_id=user_id, + updates={ + "pptxS3Key": _pptx_key, + "updatedAt": _dt.now(_tz.utc).isoformat(), + "slideCount": len(slides), + }, + ) + except Exception as e: + logger.warning("PPTX artifact refresh failed: %s", e) + + if deck_changed: # Compose: SVG → optimized JSON for WebUI animation # Only generates compose for measure_slides slugs (parallel-safe). # Uses _prepare_epoch (snapshot time) so the composer with the @@ -945,7 +1011,8 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, If style_name is provided, the style HTML is loaded as style.html. The code can read/write it via normal file I/O (open, read, write). - If save=True, style.html is written back to the user's style storage. + Writes always persist — if style.html changed, it is written back to the + user's style storage automatically. There is no "unsaved" state. If ref_styles are provided, they are downloaded and available as ref/{name}.html. Use list_styles to discover available style names. @@ -954,16 +1021,16 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, for color computation, palette extraction, and contrast calculation. Workspace layout: - style.html — target style (read/write, saved back when save=True) + style.html — target style (read/write; persisted when changed) ref/{name}.html — reference styles (read-only) Examples: Read reference: run_style_python(code="html = open('ref/corporate-executive.html').read(); print(html[:200])", ref_styles=["corporate-executive"]) Create new: run_style_python(code="open('style.html','w').write('...')", - style_name="style-20260506-1430", save=True) + style_name="style-20260506-1430") Edit existing: run_style_python(code="html = open('style.html').read(); html = html.replace('old','new'); open('style.html','w').write(html)", - style_name="style-20260506-1430", save=True) + style_name="style-20260506-1430") Compute colors: run_style_python(code="from colorsys import rgb_to_hls; print(rgb_to_hls(0.2, 0.4, 0.6))") Args: @@ -971,15 +1038,13 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, written in the user's language. Shown in the UI. code: Python code to execute. style_name: Style name to load as style.html. Optional. - save: If True, save style.html back to storage. Requires style_name. + save: Deprecated and ignored. style.html persists automatically + when changed (requires style_name). ref_styles: Style names to load as ref/{name}.html. Optional. Returns: JSON string: {"output", "saved"?} """ - if save and not style_name: - return json.dumps({"error": "save=True requires style_name"}) - user_id = _get_user_id() client = boto3.client("bedrock-agentcore", region_name=_region) @@ -993,10 +1058,12 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, try: file_contents: list[dict[str, str]] = [] - # Load target style + # Load target style (baseline for change detection) + baseline_style: str | None = None if style_name: html = _load_style_html(user_id, style_name) if html: + baseline_style = html file_contents.append({"path": "style.html", "text": html}) # Load reference styles @@ -1031,9 +1098,14 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, output = sandbox_mod._collect_stream(response) result: dict = {"output": output} + if save: + result["deprecated"] = ( + "'save' is ignored: style.html persists automatically " + "when changed." + ) - # Save style.html back to S3 - if save and style_name: + # Persist style.html when it changed (always — no "unsaved" state) + if style_name: read_code = "import sys\ntry:\n print(open('style.html').read())\nexcept FileNotFoundError:\n print('__NOT_FOUND__')\n" read_resp = client.invoke_code_interpreter( codeInterpreterIdentifier="aws.codeinterpreter.v1", @@ -1041,7 +1113,12 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, arguments={"language": "python", "code": read_code}, ) style_html = sandbox_mod._collect_stream(read_resp) - if style_html and style_html.strip() != "__NOT_FOUND__": + if ( + style_html + and style_html.strip() != "__NOT_FOUND__" + # print() appends a newline — compare newline-insensitively + and style_html.rstrip("\n") != (baseline_style or "").rstrip("\n") + ): key = f"user-styles/{user_id}/{style_name}.html" _storage.upload_file(key=key, data=style_html.encode("utf-8"), content_type="text/html") result["saved"] = {"filename": f"{style_name}.html", "key": key} diff --git a/servers/remote/tools/sandbox.py b/servers/remote/tools/sandbox.py index d0b67964..b7915091 100644 --- a/servers/remote/tools/sandbox.py +++ b/servers/remote/tools/sandbox.py @@ -75,33 +75,32 @@ def execute_in_sandbox( storage: Storage, region: str, deck_id: str | None = None, - save: bool = False, + persist_writes: bool = True, files: list[str] | None = None, -) -> tuple[str, bool]: +) -> tuple[str, list[dict], list[dict], list[str]]: """Execute Python code in Amazon Bedrock AgentCore Code Interpreter sandbox. When deck_id is provided, the entire deck workspace is loaded into the sandbox filesystem. The user code can read/write any file via normal - file I/O (open, json.load, etc.). If save=True, modified and new files - are written back to S3. + file I/O (open, json.load, etc.). Modified and new files are always + written back to S3 (diff against the session-start snapshot) — there is + no "unsaved" state. Args: code: Python code to execute. storage: Storage backend for S3 operations. region: AWS region for Code Interpreter API. deck_id: If provided, loads deck workspace into sandbox. - save: If True, writes changed files back to S3. Requires deck_id. + persist_writes: Set False to run without write-back (used when the + caller only has read access to the deck). files: Additional S3 keys to download into sandbox by basename. Returns: - Tuple of (code execution output, outline_warnings list). + Tuple of (output, outline_warnings, lint_diagnostics, changed paths). Raises: - ValueError: If save=True without deck_id, or duplicate filenames in files. + ValueError: If duplicate filenames in files. """ - if save and not deck_id: - raise ValueError("save=True requires deck_id") - if files: basenames = [key.rsplit("/", 1)[-1] for key in files] seen: set[str] = set() @@ -121,9 +120,11 @@ def execute_in_sandbox( logger.info("Code Interpreter session started: %s", session_id) try: - # Load deck workspace into sandbox + # Load deck workspace into sandbox (baseline snapshot for diff-based + # write-back) + baseline: dict[str, str] = {} if deck_id: - _upload_deck_workspace(client, session_id, storage, deck_id) + baseline = _upload_deck_workspace(client, session_id, storage, deck_id) # Inject shared sandbox helpers (read_json / write_json / ...) so user # code can use the same API on Local and Cloud. @@ -148,16 +149,22 @@ def execute_in_sandbox( ) output = _collect_stream(response) - # Save modified workspace files back to S3 + # Always write modified workspace files back to S3 — writes persist + # unconditionally (diff-based, changed/new files only). outline_warnings: list[dict] = [] lint_diagnostics: list[dict] = [] - if save and deck_id: - outline_warnings, lint_diagnostics = _save_deck_workspace( - client, session_id, storage, deck_id, + changed_paths: list[str] = [] + if deck_id and persist_writes: + outline_warnings, lint_diagnostics, changed_paths = _save_deck_workspace( + client, session_id, storage, deck_id, baseline=baseline, ) - logger.info("Deck workspace saved for deck %s", deck_id) + if changed_paths: + logger.info( + "Deck workspace saved for deck %s (%d changed files)", + deck_id, len(changed_paths), + ) - return output, outline_warnings, lint_diagnostics + return output, outline_warnings, lint_diagnostics, changed_paths finally: client.stop_code_interpreter_session( @@ -182,7 +189,8 @@ def _upload_deck_workspace( deck_id: Deck identifier. Returns: - List of relative paths written to the sandbox. + Mapping of relative path → uploaded text content (baseline snapshot + used for diff-based write-back). """ prefix = f"decks/{deck_id}/" keys = storage.list_files(prefix=prefix, bucket=storage.pptx_bucket) @@ -215,7 +223,7 @@ def _upload_deck_workspace( }, ) - return [f["path"] for f in file_contents] + return {f["path"]: f["text"] for f in file_contents} def _save_deck_workspace( @@ -223,24 +231,28 @@ def _save_deck_workspace( session_id: str, storage: Storage, deck_id: str, -) -> bool: + baseline: dict[str, str] | None = None, +) -> tuple[list[dict], list[dict], list[str]]: """Read workspace files from sandbox via prefix scan and write back to S3. Scans the sandbox for files matching _WORKSPACE_PREFIXES instead of relying on the upload paths list. This ensures newly created files (e.g., slides/{slug}.json) are automatically saved. - If specs/outline.md is present and fails lint, it is excluded from the - S3 write-back (rejected). + Diff-based: files whose content equals the baseline (what was uploaded + at session start) are skipped. This keeps the always-persist contract + cheap and prevents a stale sandbox copy from clobbering files another + writer changed on S3 in the meantime. Args: client: Bedrock AgentCore client. session_id: Code Interpreter session ID. storage: Storage backend. deck_id: Deck identifier. + baseline: Relative path → content as uploaded at session start. Returns: - True if outline.md was rejected due to lint failure, False otherwise. + Tuple of (outline_warnings, lint_diagnostics, changed relative paths). """ # Scan sandbox for all workspace files via executeCode prefixes_repr = repr(_WORKSPACE_PREFIXES) @@ -269,6 +281,12 @@ def _save_deck_workspace( file_map: dict[str, str] = json.loads(raw) + # Diff against the session-start baseline — only changed/new files are + # written back. Unchanged files are skipped so a stale sandbox copy can + # never overwrite a newer S3 write from a parallel session. + if baseline: + file_map = {p: t for p, t in file_map.items() if baseline.get(p) != t} + # Lint outline.md before saving — warn on failure outline_warnings: list[dict] = [] outline_key = "specs/outline.md" @@ -297,7 +315,7 @@ def _save_deck_workspace( except (json.JSONDecodeError, TypeError): pass - # Write back to S3 + # Write back to S3 (changed/new files only) prefix = f"decks/{deck_id}/" for rel_path, text in file_map.items(): s3_key = prefix + rel_path @@ -307,7 +325,7 @@ def _save_deck_workspace( content_type=_content_type(rel_path), ) - return outline_warnings, lint_diagnostics + return outline_warnings, lint_diagnostics, sorted(file_map.keys()) def _inject_helpers(client: Any, session_id: str) -> None: diff --git a/tests/test_agent_modes.py b/tests/test_agent_modes.py index 905aad2b..50f8725f 100644 --- a/tests/test_agent_modes.py +++ b/tests/test_agent_modes.py @@ -125,11 +125,14 @@ def test_compose_report_wiring_is_l4_delta_only(): def test_style_creator_carries_remote_sandbox_wiring(): """The style persona documents the local read_style/write_style sandbox; - Remote's run_style_python uses style_name/save/ref_styles instead, so L4 - must inject the adapter-specific I/O as wiring (review finding, PR #231). + Remote's run_style_python uses style_name/ref_styles file I/O instead, so + L4 must inject the adapter-specific I/O as wiring (review finding, PR #231). + Writes persist automatically (run-python-unified-semantics SPEC) — the + wiring must say so and must NOT teach a save flag. """ values = [p.source.value for p in MODES["style_creator"].parts if p.source.type == "file"] assert "wiring/style_remote" in values wiring_text = (_PROMPTS_DIR / "wiring" / "style_remote.md").read_text(encoding="utf-8") - for token in ("style_name", "save=True", "ref_styles"): + for token in ("style_name", "ref_styles", "persisted automatically"): assert token in wiring_text + assert "save=True" not in wiring_text diff --git a/tests/test_run_python_semantics.py b/tests/test_run_python_semantics.py new file mode 100644 index 00000000..4680d9b4 --- /dev/null +++ b/tests/test_run_python_semantics.py @@ -0,0 +1,208 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: MIT-0 +"""Semantics contract tests for run_python — Local and Remote must agree. + +The contract (decided 2026-08-01, see the run-python-unified-semantics SPEC): + +1. File writes ALWAYS persist. There is no "unsaved" state and no flag + that gates persistence. +2. The PPTX artifact rebuilds automatically whenever build-relevant files + changed (deck.json / slides/ / includes/ / specs/outline.md). +3. ``measure_slides`` is the only trigger for the expensive verification + pass (render + measure + preview). +4. The legacy ``save`` argument is accepted but ignored (deprecation note + in the result), so older callers keep working. + +These tests pin the semantics so an adapter cannot silently diverge again +(the v0.5.1 cloud E2E regression was exactly such a divergence). +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +_root = Path(__file__).resolve().parent.parent +_local = str(_root / "servers" / "local") +if _local not in sys.path: + sys.path.insert(0, _local) + +import sandbox_tools # noqa: E402 (servers/local) + + +# --------------------------------------------------------------------------- +# Local: _build_snapshot change detection +# --------------------------------------------------------------------------- + + +@pytest.fixture() +def deck_dir(tmp_path: Path) -> Path: + (tmp_path / "slides").mkdir() + (tmp_path / "specs").mkdir() + (tmp_path / "includes").mkdir() + (tmp_path / "deck.json").write_text('{"template": "t.pptx"}') + (tmp_path / "specs" / "outline.md").write_text("- [title] Hello\n") + (tmp_path / "slides" / "title.json").write_text('{"elements": []}') + return tmp_path + + +class TestBuildSnapshot: + def test_detects_slide_change(self, deck_dir: Path): + before = sandbox_tools._build_snapshot(deck_dir) + p = deck_dir / "slides" / "title.json" + p.write_text('{"elements": [{"type": "textbox"}]}') + assert sandbox_tools._build_snapshot(deck_dir) != before + + def test_detects_new_slide(self, deck_dir: Path): + before = sandbox_tools._build_snapshot(deck_dir) + (deck_dir / "slides" / "new.json").write_text("{}") + assert sandbox_tools._build_snapshot(deck_dir) != before + + def test_detects_outline_change(self, deck_dir: Path): + before = sandbox_tools._build_snapshot(deck_dir) + (deck_dir / "specs" / "outline.md").write_text("- [title] Changed\n") + assert sandbox_tools._build_snapshot(deck_dir) != before + + def test_ignores_non_build_files(self, deck_dir: Path): + before = sandbox_tools._build_snapshot(deck_dir) + (deck_dir / "specs" / "brief.md").write_text("# Brief\n") + (deck_dir / "output.pptx").write_bytes(b"x") + assert sandbox_tools._build_snapshot(deck_dir) == before + + +# --------------------------------------------------------------------------- +# Local: run_python persistence & build semantics +# --------------------------------------------------------------------------- + + +class TestLocalRunPython: + def _patch_generate(self, monkeypatch): + calls: list[dict] = [] + + def fake_generate(json_path=None, output_path=None, **kw): + calls.append({"json_path": json_path, "output_path": output_path, **kw}) + Path(output_path).write_bytes(b"pptx") + return {"output_path": str(output_path), "warnings": [], "errors": {}} + + import sdpm.api + monkeypatch.setattr(sdpm.api, "generate", fake_generate) + return calls + + def test_write_persists_and_triggers_build_without_any_flag( + self, deck_dir: Path, monkeypatch + ): + calls = self._patch_generate(monkeypatch) + out = json.loads(sandbox_tools.run_python( + purpose="write brief-independent slide", + code='write_json("slides/added.json", {"elements": []})', + deck_id=str(deck_dir), + )) + # Persistence is unconditional + assert (deck_dir / "slides" / "added.json").exists() + # Build followed the change automatically + assert len(calls) == 1 + assert "pptx" in out + + def test_readonly_run_does_not_build(self, deck_dir: Path, monkeypatch): + calls = self._patch_generate(monkeypatch) + out = json.loads(sandbox_tools.run_python( + purpose="read deck", + code='print(read_json("deck.json")["template"])', + deck_id=str(deck_dir), + )) + assert "t.pptx" in out["output"] + assert calls == [] + assert "pptx" not in out + + def test_non_build_write_persists_without_build(self, deck_dir: Path, monkeypatch): + calls = self._patch_generate(monkeypatch) + json.loads(sandbox_tools.run_python( + purpose="write brief", + code='write_text("specs/brief.md", "# Brief")', + deck_id=str(deck_dir), + )) + assert (deck_dir / "specs" / "brief.md").read_text() == "# Brief" + assert calls == [] + + def test_save_flag_is_ignored_with_deprecation_note( + self, deck_dir: Path, monkeypatch + ): + calls = self._patch_generate(monkeypatch) + out = json.loads(sandbox_tools.run_python( + purpose="read only with legacy save flag", + code='print("hi")', + deck_id=str(deck_dir), + save=True, + )) + # save no longer forces a build (nothing changed) + assert calls == [] + assert "deprecated" in out + + +# --------------------------------------------------------------------------- +# Remote: diff-based always-persist write-back +# --------------------------------------------------------------------------- + +from tools import sandbox as remote_sandbox # noqa: E402 (servers/remote via conftest) + + +class _FakeStorage: + def __init__(self): + self.uploads: dict[str, bytes] = {} + + def upload_file(self, key: str, data: bytes, content_type: str = ""): + self.uploads[key] = data + + +class TestRemoteSaveWorkspace: + def _run(self, sandbox_files: dict[str, str], baseline: dict[str, str]): + client = MagicMock() + storage = _FakeStorage() + # _save_deck_workspace reads the sandbox via _collect_stream(response) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(remote_sandbox, "_collect_stream", + lambda _resp: json.dumps(sandbox_files)) + warnings, lint, changed = remote_sandbox._save_deck_workspace( + client, "session", storage, "deckX", baseline=baseline, + ) + return storage, changed + + def test_only_changed_files_written_back(self): + baseline = {"specs/brief.md": "old", "deck.json": "{}"} + sandbox_files = { + "specs/brief.md": "new", # changed + "deck.json": "{}", # unchanged + "specs/notes.md": "created", # new + } + storage, changed = self._run(sandbox_files, baseline) + assert changed == ["specs/brief.md", "specs/notes.md"] + assert set(storage.uploads) == { + "decks/deckX/specs/brief.md", + "decks/deckX/specs/notes.md", + } + + def test_unchanged_workspace_writes_nothing(self): + baseline = {"specs/brief.md": "same"} + storage, changed = self._run({"specs/brief.md": "same"}, baseline) + assert changed == [] + assert storage.uploads == {} + + +class TestRemoteContractShape: + def test_execute_in_sandbox_has_no_save_gate(self): + import inspect + sig = inspect.signature(remote_sandbox.execute_in_sandbox) + assert "save" not in sig.parameters, ( + "execute_in_sandbox must not gate persistence on a save flag" + ) + assert sig.parameters["persist_writes"].default is True + + def test_remote_run_python_still_accepts_save_for_compat(self): + # The MCP-facing tool keeps the parameter (deprecated, ignored) so + # existing callers don't break. Read by explicit path — both servers + # ship a server.py, so module resolution is ambiguous here. + src = (_root / "servers" / "remote" / "server.py").read_text(encoding="utf-8") + assert "save: bool = False" in src + assert "'save' is ignored" in src From 702fd677f5d8a63e9fe50f984c748706e7b0964a Mon Sep 17 00:00:00 2001 From: ShotaroKataoka Date: Sat, 1 Aug 2026 13:12:08 +0900 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=90=9B=20fix(run-python-unified-seman?= =?UTF-8?q?tics):=20review=20fixes=20=E2=80=94=20verify/artifact=20separat?= =?UTF-8?q?ion,=20files=20collision=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings (all 6 addressed): - High: expensive SVG render/compose ran on deck_changed without measure_slides (contract violation), and compose/preview skipped when measuring an unchanged deck. Introduced _post_processing_plan (pure decision table: build/artifact/verify) and rewired the gates; no more measure-error noise without a measure request - High: files=[...] staged by basename could shadow deck.json and persist via the unconditional write-back — collision names now rejected - Medium: artifact refresh failures now surface as result.pptx_error with best-effort compensation delete of the orphaned upload - Medium: run_style_python warns when style.html was written without style_name (no more silent discard) - Low: import-pptx.md stale generate_pptx freshness claim updated - Low: _upload_deck_workspace return annotation fixed Added real-wiring branch tests for the 4-pattern contract matrix (monkeypatched rig around the actual run_python). SPEC: 20260801-1122_run-python-unified-semantics Progress: review fixes complete; make all 640 passed / lint clean Next: local compose smoke → cloud redeploy → user re-E2E --- sdpm/references/guides/import-pptx.md | 11 +- servers/remote/server.py | 152 +++++++++++++++------ servers/remote/tools/sandbox.py | 12 +- tests/test_run_python_semantics.py | 184 ++++++++++++++++++++++++++ 4 files changed, 309 insertions(+), 50 deletions(-) diff --git a/sdpm/references/guides/import-pptx.md b/sdpm/references/guides/import-pptx.md index ce7762e0..440599f8 100644 --- a/sdpm/references/guides/import-pptx.md +++ b/sdpm/references/guides/import-pptx.md @@ -272,12 +272,11 @@ includes every slide, followed by preview and SVG compose (triggered by `measure_slides`). The PPTX-derived placeholder template means **layout mismatch is impossible** — the build should succeed in one shot. -After the `run_python` call returns successfully, call -`generate_pptx(deck_id=deck_id)` once. This persists `output.pptx` -to the deck workspace and updates the deck record's `pptxS3Key`, so -the Web UI can offer a "Download PPTX" button immediately. Without -this call the UI sees no PPTX yet and hides the download action, -even though the slides have rendered. +The deck's PPTX artifact and the deck record's `pptxS3Key` refresh +automatically when the deck changes, so the Web UI's "Download PPTX" +action works without further steps. Call `generate_pptx(deck_id=deck_id)` +only as the final handoff — it additionally produces the WebP preview +set and returns the download link plus a full-deck warnings report. --- diff --git a/servers/remote/server.py b/servers/remote/server.py index 01af4397..11e9d87b 100644 --- a/servers/remote/server.py +++ b/servers/remote/server.py @@ -591,6 +591,38 @@ def code_to_slide(deck_id: str, code: str, name: str, # --- Code Execution (Code Interpreter) --- +def _build_relevant(p: str) -> bool: + """True if a workspace path affects the built PPTX artifact.""" + return ( + p in ("deck.json", "presentation.json", "specs/outline.md") + or p.startswith(("slides/", "includes/")) + ) + + +def _post_processing_plan(deck_changed: bool, + measure_slides: list[str] | None) -> dict[str, bool]: + """Decide run_python post-processing actions (the unified contract). + + - build: cheap python-pptx build — prerequisite for both the artifact + refresh and the verification pass + - artifact: refresh the deck's PPTX artifact (follows deck changes + automatically; failure must surface in the result) + - verify: expensive verification (measure / SVG compose / preview) — + triggered by measure_slides and ONLY by measure_slides + + Contract matrix (pinned by tests/test_run_python_semantics.py): + changed=False, measure=None → nothing + changed=True, measure=None → build + artifact only + changed=False, measure=[..] → build + verify only + changed=True, measure=[..] → build + artifact + verify + """ + return { + "build": bool(deck_changed or measure_slides), + "artifact": bool(deck_changed), + "verify": bool(measure_slides), + } + + @mcp.tool() def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = False, files: list[str] | None = None, measure_slides: list[str] | None = None) -> str: @@ -729,15 +761,10 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = # Post-processing: rebuild the PPTX artifact whenever build-relevant files # changed (the artifact follows the deck automatically); measure_slides - # additionally triggers the verification pass. - def _build_relevant(p: str) -> bool: - return ( - p in ("deck.json", "presentation.json", "specs/outline.md") - or p.startswith(("slides/", "includes/")) - ) - + # (and ONLY measure_slides) triggers the expensive verification pass. deck_changed = any(_build_relevant(p) for p in changed_paths) - if deck_id and (measure_slides or deck_changed): + plan = _post_processing_plan(deck_changed, measure_slides) + if deck_id and plan["build"]: import shutil import traceback @@ -759,43 +786,47 @@ def _build_relevant(p: str) -> bool: page_numbers = [slug_to_page[slug] for slug in (measure_slides or []) if slug in slug_to_page] page_to_slug = {v: k for k, v in slug_to_page.items()} - # Measure - try: - if page_numbers: - measure_result = _run_measure(tmpdir, pptx_path, page_numbers, page_to_slug=page_to_slug) - result["measure"] = measure_result - else: - result["measure"] = json.dumps({"error": "No matching slides found for given slugs"}) - except Exception as e: - result["measure"] = json.dumps({"error": str(e)}) + if plan["verify"]: + # Measure + try: + if page_numbers: + measure_result = _run_measure(tmpdir, pptx_path, page_numbers, page_to_slug=page_to_slug) + result["measure"] = measure_result + else: + result["measure"] = json.dumps({"error": "No matching slides found for given slugs"}) + except Exception as e: + result["measure"] = json.dumps({"error": str(e)}) - # Layout bias (filter to measured slides; bias uses 1-based) - try: - from sdpm.engine.preview import check_layout_imbalance_data - layout_bias = [b for b in check_layout_imbalance_data(pptx_path, slide_defs=slides) if b.get("slide") in set(page_numbers)] - if layout_bias: - result["warnings"] = {"layoutBias": layout_bias} - except Exception as e: - logger.warning("Layout bias check failed: %s", e) - - # Invalid-layout errors scoped to measured slugs only. Each - # composer owns a subset of slides, so leaking another group's - # mistake would be noise (they cannot fix it anyway). - measured_set = set(measure_slides or []) - my_invalids = [e for e in invalid_layouts if e.get("slug") in measured_set] - if my_invalids: - errs = result.setdefault("errors", {}) - for e in my_invalids: - errs[e["slug"]] = { - "invalidLayout": e["attempted"], - "available": e["available"], - } - - if deck_changed: + # Layout bias (filter to measured slides; bias uses 1-based) + try: + from sdpm.engine.preview import check_layout_imbalance_data + layout_bias = [b for b in check_layout_imbalance_data(pptx_path, slide_defs=slides) if b.get("slide") in set(page_numbers)] + if layout_bias: + result["warnings"] = {"layoutBias": layout_bias} + except Exception as e: + logger.warning("Layout bias check failed: %s", e) + + # Invalid-layout errors scoped to measured slugs only. Each + # composer owns a subset of slides, so leaking another group's + # mistake would be noise (they cannot fix it anyway). + measured_set = set(measure_slides or []) + my_invalids = [e for e in invalid_layouts if e.get("slug") in measured_set] + if my_invalids: + errs = result.setdefault("errors", {}) + for e in my_invalids: + errs[e["slug"]] = { + "invalidLayout": e["attempted"], + "available": e["available"], + } + + if plan["artifact"]: # Refresh the download artifact — the deck's PPTX follows deck # changes automatically (same upload/record shape as # generate_pptx; WebP previews and KB sync stay with # generate_pptx, the explicit finalize/handoff step). + # A failure here means the download artifact is STALE — that + # must be visible to the caller, not just logged. + _pptx_key = None try: import uuid as _uuid from datetime import datetime as _dt, timezone as _tz @@ -818,8 +849,22 @@ def _build_relevant(p: str) -> bool: ) except Exception as e: logger.warning("PPTX artifact refresh failed: %s", e) + result["pptx_error"] = ( + f"PPTX artifact refresh failed — the downloadable " + f"PPTX is stale. Run generate_pptx to refresh it. " + f"({e})" + ) + if _pptx_key: + # The record update may have failed after the upload + # succeeded — delete the orphaned object (best effort). + try: + _storage._s3.delete_object( + Bucket=_storage.pptx_bucket, Key=_pptx_key, + ) + except Exception: + pass - if deck_changed: + if plan["verify"]: # Compose: SVG → optimized JSON for WebUI animation # Only generates compose for measure_slides slugs (parallel-safe). # Uses _prepare_epoch (snapshot time) so the composer with the @@ -859,7 +904,8 @@ def _fp(c: dict) -> str: # Determine which slugs to generate compose for # Always include slugs that have no existing compose (migration + first build) - compose_slugs = set(measure_slides) if measure_slides else set(slug_to_page.keys()) + # Verify-gated: measure_slides is always set here + compose_slugs = set(measure_slides) for s in slug_to_page: if not _latest_key(f"{compose_prefix}{s}_"): compose_slugs.add(s) @@ -990,7 +1036,13 @@ def _fp(c: dict) -> str: pass else: logger.exception("run_python post-processing failed: deck=%s", deck_id) - result["measure"] = json.dumps({"error": msg, "traceback": traceback.format_exc()}) + if plan["verify"]: + result["measure"] = json.dumps({"error": msg, "traceback": traceback.format_exc()}) + else: + result["pptx_error"] = ( + f"PPTX build failed — the downloadable PPTX may be " + f"stale: {msg}" + ) return json.dumps(result, ensure_ascii=False) @@ -1122,6 +1174,20 @@ def run_style_python(purpose: str, code: str, style_name: str | None = None, key = f"user-styles/{user_id}/{style_name}.html" _storage.upload_file(key=key, data=style_html.encode("utf-8"), content_type="text/html") result["saved"] = {"filename": f"{style_name}.html", "key": key} + else: + # No style_name → nothing can persist. If the code wrote + # style.html anyway, that would be silent data loss — surface it. + exists_resp = client.invoke_code_interpreter( + codeInterpreterIdentifier="aws.codeinterpreter.v1", + sessionId=session_id, name="executeCode", + arguments={"language": "python", + "code": "import os\nprint(os.path.exists('style.html'))\n"}, + ) + if sandbox_mod._collect_stream(exists_resp).strip() == "True": + result["warning"] = ( + "style.html was written but no style_name was given — " + "it was NOT persisted. Re-run with style_name=." + ) return json.dumps(result, ensure_ascii=False) diff --git a/servers/remote/tools/sandbox.py b/servers/remote/tools/sandbox.py index b7915091..f827ade8 100644 --- a/servers/remote/tools/sandbox.py +++ b/servers/remote/tools/sandbox.py @@ -108,6 +108,16 @@ def execute_in_sandbox( if name in seen: raise ValueError(f"Duplicate filename: {name}") seen.add(name) + # Staged files land in the sandbox root by basename, and the + # whole workspace is persisted unconditionally afterwards — a + # name that shadows a workspace file (e.g. "deck.json") would + # silently overwrite the deck on S3. Reject it up front. + if any(name == p or name.startswith(p) for p in _WORKSPACE_PREFIXES): + raise ValueError( + f"File name {name!r} collides with the deck workspace " + "(writes always persist) — rename the file before " + "passing it via files=[...]." + ) client = boto3.client("bedrock-agentcore", region_name=region) @@ -179,7 +189,7 @@ def _upload_deck_workspace( session_id: str, storage: Storage, deck_id: str, -) -> list[str]: +) -> dict[str, str]: """Download all deck files from S3 and write them into the sandbox. Args: diff --git a/tests/test_run_python_semantics.py b/tests/test_run_python_semantics.py index 4680d9b4..2849b80e 100644 --- a/tests/test_run_python_semantics.py +++ b/tests/test_run_python_semantics.py @@ -206,3 +206,187 @@ def test_remote_run_python_still_accepts_save_for_compat(self): src = (_root / "servers" / "remote" / "server.py").read_text(encoding="utf-8") assert "save: bool = False" in src assert "'save' is ignored" in src + + +# --------------------------------------------------------------------------- +# Remote: staged files must not shadow the persisted workspace +# --------------------------------------------------------------------------- + + +class TestRemoteFilesCollision: + def test_deck_json_basename_rejected(self): + # files land in the sandbox root by basename and the workspace is + # persisted unconditionally — "deck.json" would corrupt the deck. + with pytest.raises(ValueError, match="collides with the deck workspace"): + remote_sandbox.execute_in_sandbox( + code="print(1)", + storage=_FakeStorage(), + region="us-east-1", + deck_id="d1", + files=["uploads/tmp/u/abc/deck.json"], + ) + + def test_duplicate_basename_still_rejected(self): + with pytest.raises(ValueError, match="Duplicate filename"): + remote_sandbox.execute_in_sandbox( + code="print(1)", + storage=_FakeStorage(), + region="us-east-1", + deck_id="d1", + files=["a/data.csv", "b/data.csv"], + ) + + +# --------------------------------------------------------------------------- +# Remote: run_python top-level post-processing contract (4-pattern matrix) +# --------------------------------------------------------------------------- + +import os # noqa: E402 + +os.environ.setdefault("DECKS_TABLE", "test-table") +os.environ.setdefault("PPTX_BUCKET", "test-pptx") +os.environ.setdefault("RESOURCE_BUCKET", "test-resource") + +import importlib.util # noqa: E402 + +_spec = importlib.util.spec_from_file_location( + "remote_server_under_test", _root / "servers" / "remote" / "server.py", +) +remote_server = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(remote_server) + + +class TestPostProcessingPlan: + """The contract matrix, as a pure decision table.""" + + @pytest.mark.parametrize("changed,measure,expected", [ + (False, None, {"build": False, "artifact": False, "verify": False}), + (True, None, {"build": True, "artifact": True, "verify": False}), + (False, ["a"], {"build": True, "artifact": False, "verify": True}), + (True, ["a"], {"build": True, "artifact": True, "verify": True}), + ]) + def test_matrix(self, changed, measure, expected): + assert remote_server._post_processing_plan(changed, measure) == expected + + def test_build_relevant_paths(self): + assert remote_server._build_relevant("deck.json") + assert remote_server._build_relevant("slides/title.json") + assert remote_server._build_relevant("includes/code.json") + assert remote_server._build_relevant("specs/outline.md") + assert not remote_server._build_relevant("specs/brief.md") + assert not remote_server._build_relevant("attachments/x/data.csv") + + +class _ArtifactStorage(_FakeStorage): + def __init__(self): + super().__init__() + self.deck_updates: list[dict] = [] + self.pptx_bucket = "test-pptx" + self._s3 = MagicMock() + self.fail_update = False + + def update_deck(self, deck_id, user_id, updates): + if self.fail_update: + raise RuntimeError("DynamoDB down") + self.deck_updates.append(updates) + + def list_files(self, prefix="", bucket=""): + return [] + + +@pytest.fixture() +def remote_rig(monkeypatch, tmp_path): + """Monkeypatched harness to exercise run_python's real branch wiring.""" + calls = {"prepare": 0, "build": 0, "measure": 0, "export_svg": 0} + storage = _ArtifactStorage() + + def fake_execute(code, storage, region, deck_id=None, + persist_writes=True, files=None): + return ("ok", [], [], fake_execute.changed_paths) + fake_execute.changed_paths = [] + + def fake_prepare(deck_id, user_id, storage): + calls["prepare"] += 1 + return tmp_path, [{"id": "a"}], {} + + def fake_build(tmpdir, slides, build_kwargs): + calls["build"] += 1 + p = tmp_path / "out.pptx" + p.write_bytes(b"pptx") + return p, [] + + def fake_measure(tmpdir, pptx_path, page_numbers, page_to_slug=None): + calls["measure"] += 1 + return "{}" + + def fake_export_svg(tmpdir, pptx_path): + calls["export_svg"] += 1 + return tmp_path / "measure.svg" # never created → compose skipped + + import tools.generate as gen_mod + monkeypatch.setattr(gen_mod, "_prepare_workspace", fake_prepare) + monkeypatch.setattr(gen_mod, "generate_previews", + lambda *a, **k: (_ for _ in ()).throw(RuntimeError("no lo"))) + monkeypatch.setattr(remote_server, "_build_pptx", fake_build) + monkeypatch.setattr(remote_server, "_run_measure", fake_measure) + monkeypatch.setattr(remote_server, "_export_svg", fake_export_svg) + monkeypatch.setattr(remote_server, "_storage", storage) + monkeypatch.setattr(remote_server, "_check_deck_access", lambda *a, **k: None) + monkeypatch.setattr(remote_server, "_get_user_id", lambda: "user1") + monkeypatch.setattr(remote_server.sandbox_mod, "execute_in_sandbox", fake_execute) + return fake_execute, storage, calls + + +class TestRemoteRunPythonBranching: + """Exercise the REAL run_python wiring for the 4-pattern contract matrix.""" + + def _run(self, measure_slides=None): + return json.loads(remote_server.run_python( + purpose="t", code="print(1)", deck_id="d1", + measure_slides=measure_slides, + )) + + def test_no_change_no_measure_does_nothing(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = [] + out = self._run() + assert calls == {"prepare": 0, "build": 0, "measure": 0, "export_svg": 0} + assert storage.uploads == {} and "measure" not in out + + def test_change_without_measure_builds_artifact_only(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["slides/a.json"] + out = self._run() + assert calls["build"] == 1 + # Cheap path only — no render, no measure, no measure-error noise + assert calls["measure"] == 0 and calls["export_svg"] == 0 + assert "measure" not in out + # Artifact refreshed + assert any(k.startswith("pptx/d1/") for k in storage.uploads) + assert storage.deck_updates and "pptxS3Key" in storage.deck_updates[0] + + def test_measure_without_change_verifies_only(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["specs/brief.md"] # not build-relevant + out = self._run(measure_slides=["a"]) + assert calls["build"] == 1 and calls["measure"] == 1 + assert "measure" in out + # No artifact refresh without a build-relevant change + assert not any(k.startswith("pptx/d1/") for k in storage.uploads) + + def test_change_with_measure_does_both(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["slides/a.json"] + out = self._run(measure_slides=["a"]) + assert calls["build"] == 1 and calls["measure"] == 1 + assert "measure" in out + assert any(k.startswith("pptx/d1/") for k in storage.uploads) + + def test_artifact_failure_surfaces_and_compensates(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["slides/a.json"] + storage.fail_update = True + out = self._run() + assert "pptx_error" in out + # The orphaned upload is deleted (best effort) + assert storage._s3.delete_object.called From 03db66ad23b60ddd207f434bec9709a7ad4ca19f Mon Sep 17 00:00:00 2001 From: ShotaroKataoka Date: Sat, 1 Aug 2026 13:14:19 +0900 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=90=9B=20fix(run-python-unified-seman?= =?UTF-8?q?tics):=20lint=20pass=20must=20not=20rewrite=20unchanged=20slide?= =?UTF-8?q?=20files?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unconditional rewrites on diagnostics bumped mtimes every call, falsely marking the deck as changed and rebuilding output.pptx on read-only runs (caught by the local compose one-pass check). Rewrite only when sanitization actually changed the content. SPEC: 20260801-1122_run-python-unified-semantics Progress: local compose one-pass verified (build+preview OK, no read-only rebuild) Next: cloud redeploy → user re-E2E --- servers/local/sandbox_tools.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/servers/local/sandbox_tools.py b/servers/local/sandbox_tools.py index ece3550f..c8cc7ee7 100644 --- a/servers/local/sandbox_tools.py +++ b/servers/local/sandbox_tools.py @@ -205,10 +205,14 @@ def run_python(purpose: str, code: str, deck_id: str = "", save: bool = False, for d in diags: d["slug"] = slug lint_diagnostics.extend(diags) - slide_file.write_text( - json.dumps(cleaned, ensure_ascii=False, indent=2) + "\n", - encoding="utf-8", - ) + # Rewrite only when sanitization changed the content — + # unconditional rewrites would bump mtime every call and + # falsely mark the deck as changed (auto-rebuild churn). + if cleaned != slide_data: + slide_file.write_text( + json.dumps(cleaned, ensure_ascii=False, indent=2) + "\n", + encoding="utf-8", + ) except (json.JSONDecodeError, TypeError): pass if lint_diagnostics: From 2226a23e7b9d33b4d68ab0448a3a210270086a49 Mon Sep 17 00:00:00 2001 From: ShotaroKataoka Date: Sat, 1 Aug 2026 13:38:33 +0900 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A7=AA=20test(run-python-unified-sema?= =?UTF-8?q?ntics):=20re-review=20fixes=20=E2=80=94=20lint=20regression=20t?= =?UTF-8?q?ests,=20scoped=20files=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add regression tests for the lint rewrite guard (diagnostics-only → no rewrite/no rebuild; _spAutoFit sanitize → rewrite + single rebuild) - Apply the same cleaned != slide_data guard to Remote lint (no reserialization for diagnostics-only results; byte-identical test) - Scope the staged-files collision guard to deck sessions; exact match for file entries (deck.jsonl no longer rejected), prefix match for directories; extracted _validate_staged_files for unit testing - Fix import-pptx.md 'download link' claim and remote generate_pptx docstring to match the actual return and finalize/handoff role SPEC: 20260801-1122_run-python-unified-semantics Progress: re-review conditions met; make all 644 passed / lint clean Next: cloud redeploy with this commit → user re-E2E --- sdpm/references/guides/import-pptx.md | 3 +- servers/remote/server.py | 10 ++-- servers/remote/tools/sandbox.py | 52 +++++++++++++------- tests/test_run_python_semantics.py | 70 +++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 21 deletions(-) diff --git a/sdpm/references/guides/import-pptx.md b/sdpm/references/guides/import-pptx.md index 440599f8..541cdbc1 100644 --- a/sdpm/references/guides/import-pptx.md +++ b/sdpm/references/guides/import-pptx.md @@ -276,7 +276,8 @@ The deck's PPTX artifact and the deck record's `pptxS3Key` refresh automatically when the deck changes, so the Web UI's "Download PPTX" action works without further steps. Call `generate_pptx(deck_id=deck_id)` only as the final handoff — it additionally produces the WebP preview -set and returns the download link plus a full-deck warnings report. +set, updates the final PPTX artifact, and returns a full-deck warnings +report. --- diff --git a/servers/remote/server.py b/servers/remote/server.py index 11e9d87b..cc4c5055 100644 --- a/servers/remote/server.py +++ b/servers/remote/server.py @@ -333,14 +333,18 @@ def import_attachment(source: str, deck_id: str, filename: str = "") -> str: @mcp.tool() def generate_pptx(deck_id: str) -> str: - """Generate final PPTX from presentation.json. Resolves include references automatically. - Call after slides are written to presentation.json. + """Generate final PPTX — the explicit finalize/handoff step. + + The PPTX artifact already refreshes automatically when the deck changes + (run_python post-processing); this tool additionally produces the WebP + preview set, syncs the knowledge base, and returns a full-deck warnings + report. Resolves include references automatically. Args: deck_id: The deck ID to generate PPTX from. Returns: - JSON with status and pptxS3Key. + JSON with status, slideCount, slides summary, and optional warnings. """ _check_deck_access(deck_id, action="generate_pptx") import traceback diff --git a/servers/remote/tools/sandbox.py b/servers/remote/tools/sandbox.py index f827ade8..8fd06091 100644 --- a/servers/remote/tools/sandbox.py +++ b/servers/remote/tools/sandbox.py @@ -70,6 +70,35 @@ def list_files(subdir="."): ) +def _validate_staged_files(files: list[str], deck_id: str | None) -> None: + """Validate `files=[...]` staging names before sandbox upload. + + Staged files land in the sandbox root by basename. When a deck workspace + is loaded, the whole workspace is persisted unconditionally afterwards — + a name that shadows a workspace file (e.g. "deck.json") would silently + overwrite the deck on S3, so those names are rejected. Without a deck + there is nothing to shadow, so any name is fine (general computation). + + File entries in _WORKSPACE_PREFIXES match exactly ("deck.json" is + rejected, "deck.jsonl" is not); directory prefixes match by prefix. + """ + basenames = [key.rsplit("/", 1)[-1] for key in files] + seen: set[str] = set() + for name in basenames: + if name in seen: + raise ValueError(f"Duplicate filename: {name}") + seen.add(name) + if deck_id and any( + name.startswith(p) if p.endswith("/") else name == p + for p in _WORKSPACE_PREFIXES + ): + raise ValueError( + f"File name {name!r} collides with the deck workspace " + "(writes always persist) — rename the file before " + "passing it via files=[...]." + ) + + def execute_in_sandbox( code: str, storage: Storage, @@ -102,22 +131,7 @@ def execute_in_sandbox( ValueError: If duplicate filenames in files. """ if files: - basenames = [key.rsplit("/", 1)[-1] for key in files] - seen: set[str] = set() - for name in basenames: - if name in seen: - raise ValueError(f"Duplicate filename: {name}") - seen.add(name) - # Staged files land in the sandbox root by basename, and the - # whole workspace is persisted unconditionally afterwards — a - # name that shadows a workspace file (e.g. "deck.json") would - # silently overwrite the deck on S3. Reject it up front. - if any(name == p or name.startswith(p) for p in _WORKSPACE_PREFIXES): - raise ValueError( - f"File name {name!r} collides with the deck workspace " - "(writes always persist) — rename the file before " - "passing it via files=[...]." - ) + _validate_staged_files(files, deck_id) client = boto3.client("bedrock-agentcore", region_name=region) @@ -321,7 +335,11 @@ def _save_deck_workspace( for d in diags: d["slug"] = slug lint_diagnostics.extend(diags) - file_map[rel_path] = json.dumps(cleaned, ensure_ascii=False) + # Reserialize only when sanitization changed the content — + # keeps Local/Remote semantics aligned (Local skips the + # rewrite for diagnostics-only lint results). + if cleaned != slide_data: + file_map[rel_path] = json.dumps(cleaned, ensure_ascii=False) except (json.JSONDecodeError, TypeError): pass diff --git a/tests/test_run_python_semantics.py b/tests/test_run_python_semantics.py index 2849b80e..3e1a5863 100644 --- a/tests/test_run_python_semantics.py +++ b/tests/test_run_python_semantics.py @@ -141,6 +141,57 @@ def test_save_flag_is_ignored_with_deprecation_note( assert "deprecated" in out +class TestLocalLintRewriteGuard: + """Regression: the lint pass must not rewrite files whose sanitized + content is unchanged — unconditional rewrites bumped mtimes on every + call and rebuilt output.pptx even for read-only runs (found during the + local compose one-pass check).""" + + _patch_generate = TestLocalRunPython._patch_generate + + def test_diagnostics_only_no_rewrite_no_rebuild( + self, deck_dir: Path, monkeypatch + ): + # "missing-type" produces a diagnostic but sanitize changes nothing + bad = deck_dir / "slides" / "bad.json" + bad.write_text('{"elements": [{"x": 1}]}') + mtime = bad.stat().st_mtime_ns + calls = self._patch_generate(monkeypatch) + + out = json.loads(sandbox_tools.run_python( + purpose="read only", + code='print("noop")', + deck_id=str(deck_dir), + )) + # Diagnostics are reported... + assert out.get("errors", {}).get("lintDiagnostics") + # ...but the file is untouched and no build was triggered + assert bad.stat().st_mtime_ns == mtime + assert calls == [] + + def test_sanitization_rewrites_and_rebuilds_once( + self, deck_dir: Path, monkeypatch + ): + # _spAutoFit is deprecated — sanitize removes it (content change) + auto = deck_dir / "slides" / "auto.json" + auto.write_text(json.dumps({"elements": [ + {"type": "textbox", "text": "t", "x": 1, "y": 1, "w": 10, "h": 10, + "_spAutoFit": True}, + ]})) + calls = self._patch_generate(monkeypatch) + + json.loads(sandbox_tools.run_python( + purpose="read only", code='print("noop")', deck_id=str(deck_dir))) + # Sanitization changed the file → rewrite + rebuild + assert "_spAutoFit" not in auto.read_text() + assert len(calls) == 1 + + # Second read-only run: content now stable → no further rebuild + json.loads(sandbox_tools.run_python( + purpose="read only", code='print("noop")', deck_id=str(deck_dir))) + assert len(calls) == 1 + + # --------------------------------------------------------------------------- # Remote: diff-based always-persist write-back # --------------------------------------------------------------------------- @@ -189,6 +240,16 @@ def test_unchanged_workspace_writes_nothing(self): assert changed == [] assert storage.uploads == {} + def test_diagnostics_only_slide_not_reserialized(self): + # A changed slide with diagnostics but no sanitize effect must be + # written back byte-identical (pretty formatting preserved) — + # mirrors the Local lint rewrite guard. + pretty = '{\n "elements": [\n {\n "x": 1\n }\n ]\n}' + storage, changed = self._run( + {"slides/bad.json": pretty}, {"slides/bad.json": "old"}) + assert changed == ["slides/bad.json"] + assert storage.uploads["decks/deckX/slides/bad.json"] == pretty.encode() + class TestRemoteContractShape: def test_execute_in_sandbox_has_no_save_gate(self): @@ -236,6 +297,15 @@ def test_duplicate_basename_still_rejected(self): files=["a/data.csv", "b/data.csv"], ) + def test_guard_scoped_to_deck_sessions_and_exact_names(self): + # Without a deck there is nothing to shadow — deck.json is fine + remote_sandbox._validate_staged_files(["x/deck.json"], deck_id=None) + # File entries match exactly: deck.jsonl must not be rejected + remote_sandbox._validate_staged_files(["x/deck.jsonl"], deck_id="d1") + # Exact workspace file name is rejected only with a deck loaded + with pytest.raises(ValueError, match="collides"): + remote_sandbox._validate_staged_files(["x/deck.json"], deck_id="d1") + # --------------------------------------------------------------------------- # Remote: run_python top-level post-processing contract (4-pattern matrix) From d9f0ef1aa4f186b3c1dcc5ad4ec72727a345a3f8 Mon Sep 17 00:00:00 2001 From: ShotaroKataoka Date: Sat, 1 Aug 2026 14:04:07 +0900 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=90=9B=20fix(run-python-unified-seman?= =?UTF-8?q?tics):=20delete=20superseded=20PPTX=20artifacts=20on=20refresh?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The automatic artifact refresh uploads a new UUID key per build-relevant change; one E2E session accumulated 19 orphaned PPTX objects (only the newest is referenced by the deck record). update_deck now returns the previous attribute values (ReturnValues=UPDATED_OLD) and both refresh paths (run_python auto-refresh, generate_pptx) delete the superseded artifact after a successful record update — best effort, with the existing lifecycle rules (pptx/ 90d, noncurrent 30d) as backstop. SPEC: 20260801-1122_run-python-unified-semantics Progress: E2E passed (deck 5fa63146); artifact cleanup fixed; 646 passed Next: PR → merge → v0.5.1 tag --- CHANGELOG.md | 4 ++++ servers/remote/server.py | 14 +++++++++++++- servers/remote/storage/__init__.py | 8 ++++++-- servers/remote/storage/aws.py | 11 +++++++++-- servers/remote/tools/generate.py | 10 +++++++++- tests/test_run_python_semantics.py | 20 ++++++++++++++++++++ 6 files changed, 61 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 502f3d61..21358946 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,10 @@ Entries before v0.5.0 were written retroactively as summaries. ### Fixed +- Cloud: superseded PPTX artifacts are now deleted after each refresh — the + automatic artifact refresh no longer accumulates orphaned objects in S3 + (`update_deck` returns previous values via `UPDATED_OLD`). + - **Cloud agent output-token limit**: model profiles now set an explicit `max_tokens` (Claude 32768, others 8192) — Bedrock's small default truncated long single-call outputs (e.g. writing `specs/brief.md` from a long article) diff --git a/servers/remote/server.py b/servers/remote/server.py index cc4c5055..f3d39a99 100644 --- a/servers/remote/server.py +++ b/servers/remote/server.py @@ -843,7 +843,7 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = ".presentationml.presentation" ), ) - _storage.update_deck( + _old = _storage.update_deck( deck_id=deck_id, user_id=user_id, updates={ "pptxS3Key": _pptx_key, @@ -851,6 +851,18 @@ def run_python(purpose: str, code: str, deck_id: str | None = None, save: bool = "slideCount": len(slides), }, ) + # The record now points at the new artifact — delete the + # superseded one so auto-refresh doesn't accumulate + # orphaned PPTX objects (best effort; lifecycle rules + # are the backstop). + _old_key = (_old or {}).get("pptxS3Key") + if _old_key and _old_key != _pptx_key: + try: + _storage._s3.delete_object( + Bucket=_storage.pptx_bucket, Key=_old_key, + ) + except Exception: + pass except Exception as e: logger.warning("PPTX artifact refresh failed: %s", e) result["pptx_error"] = ( diff --git a/servers/remote/storage/__init__.py b/servers/remote/storage/__init__.py index 825d0fe9..6a4d142b 100644 --- a/servers/remote/storage/__init__.py +++ b/servers/remote/storage/__init__.py @@ -27,8 +27,12 @@ def get_deck(self, deck_id: str, user_id: str) -> Optional[dict]: """Get deck metadata. Returns None if not found.""" @abstractmethod - def update_deck(self, deck_id: str, user_id: str, updates: dict) -> None: - """Partial update of deck metadata.""" + def update_deck(self, deck_id: str, user_id: str, updates: dict) -> dict: + """Partial update of deck metadata. + + Returns: + Previous values of the updated attributes (may be empty). + """ # --- Presentation JSON (S3) --- diff --git a/servers/remote/storage/aws.py b/servers/remote/storage/aws.py index 4f499609..4215633d 100644 --- a/servers/remote/storage/aws.py +++ b/servers/remote/storage/aws.py @@ -59,7 +59,7 @@ def get_deck(self, deck_id: str, user_id: str) -> Optional[dict]: resp = self._table.get_item(Key={"PK": f"USER#{user_id}", "SK": f"DECK#{deck_id}"}) return resp.get("Item") - def update_deck(self, deck_id: str, user_id: str, updates: dict) -> None: + def update_deck(self, deck_id: str, user_id: str, updates: dict) -> dict: """Partial update of deck metadata in DDB. Keys with None values are removed from the item (REMOVE expression). @@ -69,6 +69,11 @@ def update_deck(self, deck_id: str, user_id: str, updates: dict) -> None: deck_id: Deck identifier. user_id: User identifier. updates: Dict of attribute names to values. None means remove. + + Returns: + Previous values of the updated attributes (UPDATED_OLD) — lets + callers clean up superseded S3 objects (e.g. the old pptxS3Key) + without an extra read. """ set_parts = [] remove_parts = [] @@ -92,7 +97,9 @@ def update_deck(self, deck_id: str, user_id: str, updates: dict) -> None: } if values: kwargs["ExpressionAttributeValues"] = values - self._table.update_item(**kwargs) + kwargs["ReturnValues"] = "UPDATED_OLD" + response = self._table.update_item(**kwargs) + return response.get("Attributes", {}) # --- Presentation JSON (S3) --- diff --git a/servers/remote/tools/generate.py b/servers/remote/tools/generate.py index 2bffe0c3..0924ae45 100644 --- a/servers/remote/tools/generate.py +++ b/servers/remote/tools/generate.py @@ -298,9 +298,17 @@ def generate_pptx( # Update deck record deck = storage.get_deck(deck_id, user_id) now = datetime.now(timezone.utc).isoformat() - storage.update_deck(deck_id=deck_id, user_id=user_id, updates={ + old = storage.update_deck(deck_id=deck_id, user_id=user_id, updates={ "pptxS3Key": pptx_key, "updatedAt": now, "slideCount": len(slides), }) + # Delete the superseded artifact (best effort) — every refresh + # uploads a new UUID key and only the newest is referenced. + old_key = (old or {}).get("pptxS3Key") + if old_key and old_key != pptx_key: + try: + storage._s3.delete_object(Bucket=storage.pptx_bucket, Key=old_key) + except Exception: + pass # Preview: epoch-keyed WebP (background) slugs = [s.get("id") or f"slide_{i + 1:02d}" for i, s in enumerate(slides)] diff --git a/tests/test_run_python_semantics.py b/tests/test_run_python_semantics.py index 3e1a5863..66b65fdd 100644 --- a/tests/test_run_python_semantics.py +++ b/tests/test_run_python_semantics.py @@ -354,11 +354,14 @@ def __init__(self): self.pptx_bucket = "test-pptx" self._s3 = MagicMock() self.fail_update = False + self.previous_pptx_key: str | None = None def update_deck(self, deck_id, user_id, updates): if self.fail_update: raise RuntimeError("DynamoDB down") self.deck_updates.append(updates) + # UPDATED_OLD semantics: previous values of the updated attributes + return {"pptxS3Key": self.previous_pptx_key} if self.previous_pptx_key else {} def list_files(self, prefix="", bucket=""): return [] @@ -460,3 +463,20 @@ def test_artifact_failure_surfaces_and_compensates(self, remote_rig): assert "pptx_error" in out # The orphaned upload is deleted (best effort) assert storage._s3.delete_object.called + + def test_superseded_artifact_deleted_after_refresh(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["slides/a.json"] + storage.previous_pptx_key = "pptx/d1/old-artifact.pptx" + out = self._run() + assert "pptx_error" not in out + # Auto-refresh must not accumulate orphaned PPTX objects + storage._s3.delete_object.assert_called_once_with( + Bucket="test-pptx", Key="pptx/d1/old-artifact.pptx") + + def test_first_artifact_deletes_nothing(self, remote_rig): + fake_execute, storage, calls = remote_rig + fake_execute.changed_paths = ["slides/a.json"] + storage.previous_pptx_key = None + self._run() + assert not storage._s3.delete_object.called