Bounded Envelope + Pull Architecture for Delivery-Bound Recipes - #4312
Closed
Trecek wants to merge 25 commits into
Closed
Bounded Envelope + Pull Architecture for Delivery-Bound Recipes#4312Trecek wants to merge 25 commits into
Trecek wants to merge 25 commits into
Conversation
…iation) Adds test_capability_fail_closed_on_missing_ctx in tests/server/test_track_response_size.py — when _get_ctx_or_none() returns None and the handler returns an 80KB payload, the decorator must fail closed via bounded_response_budget_failure(cause='context_unavailable') rather than passing the payload through unbounded. Mirrors the mocking pattern of the sibling test_capability_fail_closed_on_missing_backend but returns None itself — the branch the sibling cannot reach. Will turn green once the backend_inspected gate is removed from track_response_size.
…ediation) The gate prevented the worst-case fallback from running when ctx is None, leaving effective_delivery_token_limit as None — which enforce_response_budget treats as an explicit opt-out, so the delivery bound was silently skipped and an oversized payload could pass through unbounded. Removing the gate makes the fallback check run unconditionally, covering BOTH fail-open paths: - ctx is None (previously fail-open) - caps not a BackendCapabilities instance (already working) The fallback body (logger.warning message + assignment) is unchanged; the flag is removed entirely to avoid dead state. shape_execution_response in server/tools/_execution_helpers.py is NOT touched — its tool_ctx parameter is required/non-Optional, so it has no ctx is None path. Closes REQ-013 in Part A.
…t (Part A)
The two starvation tests previously never exercised the populated
post_prune_step_names path — _delivery_starvation_payload never inserted
the key, making the test_delivery_bound_summary_budget_reallocation_after_projection
payload.pop("post_prune_step_names", None) line a no-op against a key that
was never present.
Changes:
- _delivery_starvation_payload gains an optional post_prune_step_names
parameter; when not None, the key is included in the payload
- _STARVATION_STEP_NAMES module-level constant lists the step_0..step_199
names generated by the helper's content skeleton
- Both tests are parametrized over [None, _STARVATION_STEP_NAMES] with
ids 'without_post_prune_step_names' / 'with_post_prune_step_names'
- WITH variants assert every supplied step name appears in delivered
content (matches the contract-level assertion style in
tests/contracts/test_delivery_bound_fitness.py)
- WITHOUT variants preserve the existing content_floor=0 fallback assertions
- The misleading no-op payload.pop line is removed
Closes REQ-025 / REQ-007 coverage gap.
…ase fallback The Part A remediation removed the backend_inspected gate so the worst-case fallback now fires when ctx is None (REQ-013 closure). Previously, open_kitchen direct-call tests (those that pass a mock_ctx directly while patching only server._get_ctx) relied on track_response_size seeing ctx=None and silently letting the unbounded response through. The new fallback now catches this case and returns a bounded_response_budget_failure envelope (cause=context_unavailable), causing seven pre-existing tests to fail with KeyError on parsed['content']/parsed['success']. Add a conftest helper _make_track_response_ready_mock_ctx that wires mock_ctx.backend.capabilities to a real BackendCapabilities with a high effective_delivery_token_limit, and update the seven direct-call tests to use the helper plus patch autoskillit.server._notify._get_ctx_or_none (in addition to server._get_ctx) so track_response_size sees the same ctx as open_kitchen. The seven tests now exercise the production path (backend capabilities resolve an effective delivery bound) instead of the previously-incidentally-open fail-open path.
open_kitchen / load_recipe now return a compact step-skeleton envelope (step_flow_skeleton + routing edges + step_index + artifact_path + sha256 + pull_tool) that fits the smallest backend delivery bound by construction. The full recipe content is persisted as a deterministic-path artifact with sha256 verification, and get_recipe_section (a new bounded pull tool) reads it on demand — closing ADR-0005 Accepted Gap #6 ("caller needing pruned collection members must retrieve bounded slices from that artifact"). Key changes: - src/autoskillit/server/_response_budget.py: add build_recipe_envelope, extract_step_routing, deterministic _artifact_path scheme (sha256-based rather than uuid4), preserved-keys extension for envelope fields. - src/autoskillit/server/tools/tools_recipe.py: persist full recipe artifact at the tool layer, return envelope via render_served_response, register get_recipe_section MCP tool with full never-raises contract and sha256 verification on the artifact (recreates from load_and_validate if missing). - src/autoskillit/server/tools/_serve_helpers.py: build_and_record_recipe_envelope consolidates the persist + envelope + ctx state update in one helper. - src/autoskillit/core/types/_type_constants_registries.py: add get_recipe_section to GATED_TOOLS, SERVE_SURFACES (5th surface), and TOOL_SUBSET_TAGS. - src/autoskillit/pipeline/context.py: add recipe_artifact_state field for server-side session cache of artifact_path/sha256/recipe-load params. - tests/: update all tests expecting the old full-payload shape; bump kitchen tool count, MCP tool count, and doc references to 60. - docs/: bump MCP tool count references from 59 to 60. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rding Restores persistence in enforce_response_budget's exempted-tool sub-branch (c) with pre-publication detection: reuses the tool-layer artifact when the payload already carries artifact_path/sha256, persists otherwise when an artifact dir is available, and skips gracefully when it is not. Threads the pre-published pair into _spill_for_delivery_bound to avoid a double write. Also drops build_recipe_envelope's diagram forwarding — diagram retrieval is exclusively via get_recipe_section(section="diagram").
Both open_kitchen recipe-bearing return paths (deferred-recall and normal) now route through a single _render_open_kitchen_envelope helper that calls build_and_record_recipe_envelope, mirroring load_recipe's existing wiring. The bare-open (name=None) path is untouched. Also fixes recreation for open_kitchen-originated artifacts: _recreate_artifact now reapplies build_open_kitchen_recipe_payload before hashing, so a regenerated artifact byte-matches the originally persisted open_kitchen payload (which carries success/kitchen/version keys the raw serve_recipe() result lacks). Without this, get_recipe_section's missing-artifact recreation path always mismatched for open_kitchen sessions.
open_kitchen(name=...) now returns a compact envelope instead of inline content, so every affected mock ToolContext needs a real temp_dir for artifact persistence, and content-reading assertions now go through the persisted artifact (or the new _open_kitchen_content helper mirroring _load_recipe_content) instead of indexing "content" directly.
- test_response_backstop.py: artifact-always-persisted, deterministic-path, and pre-published-not-rewritten tests for the exempted-branch backstop (Tests A1, A2, A2b). - test_tools_recipe.py: TestGetRecipeSection exercises get_recipe_section after an open_kitchen-only session — step resolution, sha256 integrity verification, and missing-artifact recreation incl. its failure path (Tests A3-A5). - test_delivery_bound_fitness.py: envelope-fits-by-construction, step_index coverage, and largest-step pull-bound fitness for every bundled recipe (Tests A6-A8); reworks the old fits-or-spills test into a fits-only per-backend contract (Test A9).
…chitecture Wiring open_kitchen's recipe-bearing paths through build_and_record_recipe_envelope (previous commit) exposed several latent gaps only reachable once open_kitchen persists artifacts and returns a compact envelope like load_recipe already did: - OpenKitchenResult (recipe/_recipe_ingredients.py) didn't declare the five compact-envelope fields (step_flow_skeleton, step_index, artifact_path, sha256, pull_tool), so the wire response carried undeclared keys. - The pretty_output open_kitchen formatter's field-coverage contract needed the same five fields classified (suppressed pending Part B's dedicated formatter work). - Many tests built mock ToolContexts without a real temp_dir, so envelope persistence raised and open_kitchen's outer exception handler turned would-be successes into kitchen:"failed" envelopes (test_mcp_overrides, test_open_kitchen_deferred_recall, test_pipeline_tracker, plus two more sites in test_tools_kitchen_visibility). - A handful of tests read fields (post_prune_step_names, warnings, valid, orchestration_rules absence) that live on the persisted artifact or aren't part of the envelope schema at all, rather than on the compact envelope. - test_serve_idempotence's deferred-recall test asserted artifact identity between two calls that legitimately pass different overrides (producing a legitimate warnings-key difference) — narrowed to the content-identity assertion that's actually guaranteed. - test_schema_version_convention's allowlist and test_subpackage_isolation's line-limit exemption needed updating for tools_kitchen.py's shifted line numbers and slightly larger size.
build_recipe_envelope raised unconditionally when the naive full-fidelity envelope exceeded the delivery bound. Large bundled recipes (implementation, implementation-groups, remediation) hit this once open_kitchen was wired through the envelope path, since their suggestions/orchestration_rules/etc. push the naive envelope past the smallest backend's bound. Advisory (non-structural) preserved fields now degrade via the existing _project_value/_minimal_same_type primitives before failing closed; step_flow_skeleton and step_index (required for full step-pull coverage) are never touched.
Extends test_delivery_bound_summary_preserves_operational_fields with the 5 Part B envelope keys (step_flow_skeleton, step_index, pull_tool, artifact_path, sha256) that _DELIVERY_BOUND_PRESERVED_KEYS already protects, so a regression dropping one of them from the spill projection path is now caught (REQ-126 test-assertion half). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…king file tool_name="open_kitchen" is exempted, so enforce_response_budget treats a payload-supplied artifact_path+sha256 pair as already published by the tool layer and reuses it verbatim as the spill metadata's artifact_path instead of persisting `original` fresh (see test_recipe_artifact_pre_published_not_rewritten). The round-3 plan's fictional sentinel path was never written to disk, so the test's own pre-existing artifact round-trip assertion failed with FileNotFoundError. Point the sentinel at a real file under tmp_path and write `original` there before invoking enforce_response_budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… offset Character-slicing json.dumps(list) then json.loads()-ing the substring produced invalid JSON on nearly every chunk boundary, silently discarding content for oversized get_recipe_section(section="suggestions") pulls while still returning success=True.
tools_recipe.py maintained a private byte-for-byte copy of _serve_helpers.resolve_envelope_delivery_bound (both added in the same commit). Import the shared helper instead of two independently maintained copies of the fallback/multiplier logic.
… strips content step_index was built unconditionally from post_prune_step_names, but ingredients_only=True strips "content" from the persisted artifact, so get_recipe_section(section="step", ...) always fails step_not_found for those steps despite step_index claiming they're pullable. Guard it the same way step_flow_skeleton already is.
…ion type Raising a bare ValueError meant load_and_validate's generic except ValueError handler reclassified an internal sizing-invariant failure as "Invalid recipe structure: ...", conflating it with legitimate recipe-authoring errors — and the fallback _bounded_append call inside that handler could raise the same ValueError unhandled. TruncationBoundsError is not caught by that handler, so it now propagates as a genuine internal error instead.
test_kitchen_tagged_tool_count_is_40 and test_docs_state_59_mcp_tools asserted 41/60 but kept their old names from before the bounded-envelope count bump, making the test names misleading.
…ddition _api.py's TruncationBoundsError import shifted the pre-existing lazy-registry suppression from line 335 to 336.
…a truncation sentinel _bounded_append's merge branch stripped the old sentinel via `target[:-1]` but never concatenated the newly appended `incoming` items into `candidates`, so once a recipe load tripped the bound once, every subsequent stage's errors/suggestions for that load were silently dropped from the list entirely while `original_count` still reported them as counted.
…eation matches the persisted artifact recipe_artifact_state never recorded whether the original load used ingredients_only=True, so _recreate_artifact always re-served the full unstripped payload. The recreated sha256 then deterministically mismatched the recorded sha256 for any artifact originally persisted from an ingredients_only=True load, so get_recipe_section always failed with artifact_recreation_mismatch instead of recreating.
track_response_size and shape_execution_response each independently implemented the same "resolve effective delivery limit from backend capabilities, fall back to the worst-case bound, log if falling back" sequence (a third copy in tools_recipe.py was already consolidated by 83b3c82). Extract resolve_effective_or_fallback_delivery_bound into _response_budget.py, which both modules already import from, and have both call sites use it instead of maintaining independent copies.
Trecek
commented
Jul 22, 2026
Trecek
left a comment
Trecek
left a comment
Collaborator
Author
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: approved_with_comments
…sertions in load_recipe ingredients_only test
Collaborator
Author
|
Superseded by #4308, which landed the bounded envelope + pull architecture on develop first. Branch preserved for reference. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Across this branch, the delivery-bound recipe summarization path was hardened in two major passes: Part A fixed the
_delivery_bound_summarybudget-priority bug (which was starvingcontentto zero bytes), boundedsuggestionsat source, closed capability fail-open holes (including thectx is Nonegap), and added the missing negative-contract and parametrized tests. Part B then eliminated the underlying structural fragility by replacing inline recipe delivery with a bounded envelope + pull architecture —open_kitchen/load_recipenow return a compact, by-construction-bounded envelope with routing metadata, backed by a persisted, sha256-verified artifact retrievable via a new pull tool. Two follow-up remediation rounds closed the gaps this re-architecture initially left open — wiringopen_kitcheninto the envelope path, restoring unconditional artifact persistence on the exempted-tool backstop, adding the 8 required TDD tests, dropping an errantdiagramfield from the envelope, and finally extendingtest_delivery_bound_summary_preserves_operational_fieldsto assert all 5 new Part B envelope keys — leaving the full suite green (31371 passed) with no known structural or test-coverage gaps remaining.Individual Group Plans
Group 1: Rectify: Delivery-Bound Summary Self-Starvation and Safety Net Gaps (Part A)
The
_delivery_bound_summaryfunction (server/_response_budget.py:375-511) starvescontentto zero bytes because preserved keys (especiallysuggestionsat 48,262 bytes) consume the entire budget beforecontentgets any allocation, and the fallback that shrinks preserved keys never reallocates freed budget back tocontent. Additionally,suggestionsaccumulates unboundedly at source (recipe/_api.py:324), capability resolution silently fails open when backend context is unavailable, and no test asserts minimum content coverage in the summary. This part fixes the budget algorithm, bounds suggestions at source, closes capability holes, and adds the missing negative contracts.Part B will cover the bounded envelope + pull architecture for full structural immunity — implement as a separate task.
Group 2: Close the ctx-is-None Delivery-Bound Fail-Open Gap and Make the post_prune_step_names Test Contrast Real (Part A Remediation)
This plan remediates the two audit findings from
/audit-implagainst the Part A rectification branch (commite55a08917, "Rectify delivery-bound summary self-starvation (Part A)"):REQ-013 gap (source fix):
track_response_sizeinsrc/autoskillit/server/_notify.pystill fails open whenctx is None. The wrapper initializesbackend_inspected = Falseand only sets itTrueinsideif ctx is not None:; the worst-case fallback block is gated onbackend_inspected, so whenctxisNonethe fallback never runs,effective_delivery_token_limitstaysNone, andenforce_response_budgettreatsNoneas "no delivery bound" — enforcement is silently skipped. The fix removes thebackend_inspectedgate entirely so the fallback check runs unconditionally, covering BOTH fail-open paths (ctx is Noneand caps-not-BackendCapabilities) with the sameresolve_worst_case_delivery_bound()+logger.warningfallback.REQ-025 / REQ-007 gap (test fix):
_delivery_starvation_payload()intests/server/test_response_backstop.pynever includes apost_prune_step_nameskey, so the starvation and budget-reallocation tests only ever exercise the empty-listcontent_floor = 0path. The fix parametrizes both tests over a with/withoutpost_prune_step_namescontrast so each test exercises BOTH the[]fallback path (REQ-025) and the populated skeleton-floor path, with step-name-presence assertions in the WITH variant.A new regression test (
test_capability_fail_closed_on_missing_ctx) captures thectx is Nonefail-open gap the existingtest_capability_fail_closed_on_missing_backendmisses.Scope boundary (REQ-034): Part B (bounded envelope + pull architecture) remains out of scope and must not be implemented under this plan.
Group 3: Rectify: Bounded Envelope + Pull Architecture for Recipe Delivery (Part B)
Even with Part A's budget-priority fix and suggestions bounding, inline recipe delivery remains structurally fragile: the remediation recipe's step section (~24.3K tokens) is 2.4× the Codex code-mode ceiling, and any recipe growth risks re-triggering starvation. This part eliminates the fragility by re-architecting recipe delivery as "bounded envelope + guaranteed pull access":
open_kitchen/load_recipereturn a compact envelope (step skeleton + routing metadata) that fits the smallest supported delivery bound by construction, and full step content is retrieved on demand via a new MCP pull tool. The full recipe is always available as a persisted artifact with a deterministic path and sha256 verification.Part A covers the bounded-summary priority fix, suggestions bounding, and capability hole closure — that part must be implemented first.
Group 4: Part B Remediation — Bounded Envelope + Pull Architecture Gaps (Part A of the gaps plan)
This part closes the structural gaps found by
/audit-implin the Part B (bounded envelope + pull architecture) implementation on this branch:open_kitchenwas never wired to the envelope architecture — both recipe-bearingserve_recipe()paths still returned the full payload without persisting an artifact, settingToolContext.recipe_artifact_state, or building the compact envelope.enforce_response_budget's exempted-tool sub-branch (c) does not persist the artifact — the unconditional-persistence requirement was implemented and then reverted; this plan restores it with pre-publication detection so a backstop persist never orphans the tool-layer artifact.build_recipe_envelopeforwardeddiagraminto the envelope despite the plan's requirement that the envelope must NOT include adiagramfield; resolved by dropping the forwarding.Part B of this plan (separate task, requires explicit authorization) covers the presentation and documentation surface: CLI prompt contract, sous-chef SKILL.md, formatter envelope handling, exemption-registry semantic docstring, and the ADR-0005 five-layer update.
Already satisfied on this branch (not re-implemented):
build_recipe_envelope,extract_step_routing, deterministic_artifact_path,build_and_record_recipe_envelope,persist_recipe_artifact,ToolContext.recipe_artifact_state, the completeget_recipe_sectiontool,load_recipewiring,SERVE_SURFACESS5 entry,_DELIVERY_BOUND_PRESERVED_KEYSenvelope fields, serve-surface arch-test expectations, and_serve_helpers.py's "five serve surfaces" docstring.Group 5: Add the 5 Part B Envelope-Key Assertions to
test_delivery_bound_summary_preserves_operational_fields(REQ-126, Round 3)Round-2
/audit-implconfirmed all six of the previous gaps plan's deliverables are satisfied andtask test-checkpasses (31371 passed, 610 skipped, 49 xfailed). Exactly one MISSING finding survives: the test-assertion half of REQ-126 was never implemented._DELIVERY_BOUND_PRESERVED_KEYScorrectly contains all 5 Part B envelope keys (step_flow_skeleton,step_index,pull_tool,artifact_path,sha256) — the registry half of REQ-126 is done. Buttest_delivery_bound_summary_preserves_operational_fieldsstill constructed its payload with, and asserted, only the original 9 keys, so a regression that drops one of the 5 new keys from the_delivery_bound_summaryspill-projection path would go undetected.This plan makes exactly one change: extend that single test's payload and its per-key assertions to cover the 5 new keys, mirroring the existing 9-key pattern, with each new key bound to a distinct, order-detectable sentinel value. No production code changes.
Closes #4305
Implementation Plan
Plan files:
/home/talon/projects/autoskillit-runs/remediation-20260720-183035-937540/.autoskillit/temp/rectify/rectify_delivery_bound_starvation_2026-07-20_183500_part_a.md/home/talon/projects/autoskillit-runs/remediation-20260720-183035-937540/.autoskillit/temp/make-plan/remediation_delivery_bound_starvation_part_a_fixes_plan_2026-07-21_071341.md/home/talon/projects/autoskillit-runs/remediation-20260720-183035-937540/.autoskillit/temp/rectify/rectify_delivery_bound_starvation_2026-07-20_183500_part_b.md/home/talon/projects/autoskillit-runs/remediation-20260720-183035-937540/.autoskillit/temp/make-plan/remediation_delivery_bound_starvation_part_b_gaps_plan_2026-07-21_140852_part_a.md/home/talon/projects/autoskillit-runs/remediation-20260720-183035-937540/.autoskillit/temp/make-plan/remediation_delivery_bound_starvation_partB_gaps_round3_plan_2026-07-21_161951.md🤖 Generated with Claude Code via AutoSkillit
Token Usage Summary
* Step used a non-Anthropic provider; caching behavior may differ.
Token Efficiency
Model Usage Breakdown