Implementation Plan: Serve Surface Symmetry — SERVE_SURFACES __all__ Alphabetical Fix#4210
Conversation
Proves open_kitchen + load_recipe content diverge without the session_serve_overrides snapshot. Five tests cover: - content equality with overrides (issue_url) - content equality without overrides (defer_unresolved path) - deferred-recall path identity - snapshot lifecycle (set on open, cleared on close) - explicit load_recipe overrides layer on top of session baseline Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…metry (part A) Eliminates overlay divergence when load_recipe or deferred-recall open_kitchen is called after an open_kitchen(overrides=...) invocation. Changes: - core/types: add ServeOverridesSnapshot Protocol for doc-layer naming - pipeline/context: add session_serve_overrides (dict[str, str] | None) and session_serve_defer_unresolved (bool) fields to ToolContext - server/_factory: wire session_serve_overrides=None in make_context() - tools_kitchen (normal path): SET snapshot after preflight, before _validate_result; store dict(overrides) not _merged_overrides to avoid injecting stale infra values into subsequent serves - tools_kitchen (_close_kitchen_handler): CLEAR snapshot to None/False alongside active_recipe_* resets - tools_kitchen (shared _merged_overrides): inject session_serve_overrides at position 3 (after _session_overrides, before explicit overrides); no-op for normal path (snapshot is None before preflight window) - tools_kitchen (deferred-recall): fix defer_unresolved=False hardcode to use session_serve_defer_unresolved from snapshot - tools_recipe: inject _session_baseline from snapshot at position 3; thread defer_unresolved from session_serve_defer_unresolved Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n (part A) - Fix __init__.pyi import ordering: ServeOverridesSnapshot alphabetically after RetryReason - Fix context.py: use ServeOverridesSnapshot Protocol annotation (not bare dict) - Fix tools_recipe.py: dict() cast avoids mypy Protocol assignment mismatch - Fix test_type_protocol_shards: add ServeOverridesSnapshot to expected recipe shard __all__ - Fix test_schema_version_convention: update legacy JSON write line numbers (1124→1137, 1184→1197) - Fix test_cascade_map_guard: add server/test_serve_idempotence.py to recipe cascade entry - Fix test_subpackage_isolation: raise tools_kitchen.py line limit exemption to 1410 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…istry (part A) Extract serve_recipe() as the single legal caller of load_and_validate in server/tools/. All four surfaces (open_kitchen normal, open_kitchen deferred-recall, load_recipe, get_recipe) now route through serve_recipe(), which owns the override stack assembly and defer_unresolved resolution. Key changes: - New _serve_helpers.py: _build_serve_override_stack, _resolve_serve_defer_unresolved, serve_recipe — the only legal call site for load_and_validate in server/tools/ - Fix get_recipe MCP resource: now reads ctx.session_serve_overrides snapshot, fixing the HIGH severity gap where caller overrides were silently dropped - Fix deferred-recall open_kitchen: updates session_serve_overrides when explicit overrides are provided, closing a routing divergence with subsequent load_recipe calls - Add SERVE_SURFACES frozenset to _type_constants_registries.py, exported via core/__init__.pyi - New tests/arch/test_serve_surface_registry.py: AST guard that load_and_validate is only called from _serve_helpers.py; SERVE_SURFACES membership completeness - New Hypothesis property test: test_load_recipe_routing_matches_open_kitchen_for_arbitrary_overrides - Mechanical: file-count (+_serve_helpers.py), line-budget update, cascade registration, schema-version allowlist, symbol-count increment (94→95) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move "SERVE_SURFACES" before "SkillCapabilityDef" in __all__ of _type_constants_registries.py so entries are alphabetically ordered (SERVE < SKILL). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…skew flake NTP clock corrections in CI can cause time.time() to return a value slightly earlier than started_at (captured before spawn), making ended_at < started_at in the dispatch DispatchRecord. The guard max(time.time(), started_at + 1e-6) ensures the invariant holds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n_overrides in get_recipe
…thesis.assume for filtered examples
…ine numbers after tools_kitchen.py fix
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
| assert first_content == deferred_content, ( | ||
| "Deferred-recall open_kitchen content diverges from first serving — " | ||
| "session_serve_overrides not injected into deferred-recall _merged_overrides" | ||
| ) |
There was a problem hiding this comment.
[warning] tests: assume(False) silently discards examples where open_kitchen fails. If _open_kitchen_patched consistently fails due to a regression, Hypothesis exhausts its examples with all discarded and reports a passing test. Replace with a hard assertion to make failures visible.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Prior review round explicitly requested assume(False) for invalid combos (commit 1f5a656). Current review correctly notes the false-pass risk when open_kitchen fails due to a regression rather than an invalid input combo. The choice between assume() (silently discards) vs. a hard assertion (fails on regression) requires a human decision about the expected failure mode.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Prior review round explicitly requested assume(False) for invalid combos (commit 1f5a656). Current review correctly notes the false-pass risk when open_kitchen fails due to a regression rather than an invalid input combo. The choice between assume() (silently discards) vs. a hard assertion (fails on regression) requires a human decision about the expected failure mode.
| ) | ||
|
|
||
|
|
||
| async def test_session_serve_overrides_cleared_on_close_kitchen( |
There was a problem hiding this comment.
[warning] tests: assume(False) on load_recipe failure silently discards examples where the system under test is broken — if load_recipe always fails (regression), the test passes vacuously. Use a hard assertion instead.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Same trade-off as the open_kitchen case: assume(False) silently discards examples where load_recipe fails, which would mask a regression. The previous review round introduced this pattern. Human decision needed on whether to switch to a hard assertion.
There was a problem hiding this comment.
Valid observation — flagged for design decision. Same trade-off as the open_kitchen case: assume(False) silently discards examples where load_recipe fails, which would mask a regression. The previous review round introduced this pattern. Human decision needed on whether to switch to a hard assertion.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: 1 critical blocking issue found + 12 warnings.
Verdict: changes_requested (posted as COMMENT — own-PR guard active)
Summary
- [critical] defense
src/autoskillit/server/tools/_serve_helpers.py:95—assertstripped by Python-O, replace with explicitRuntimeError - 12 warnings across defense, tests, arch, slop, and bugs dimensions — see inline comments
…safety assert is stripped by Python -O, leaving a bare AttributeError with no diagnostic context when ctx.recipes is None in optimized builds.
…pshot
dict(overrides) if overrides else {} inside if overrides is not None: is
redundant — both branches produce dict(overrides). Simplify to dict(overrides).
…ne comment The inline comment duplicated the docstring prose. Consolidate into docstring so the TODO is co-located with the rationale text.
- Replace fragile startswith('{') heuristic with json.loads check (3 sites)
- Remove hardcoded steps['clone']['on_success'] assertion that couples test
to recipe internals (content equality assertion already covers this)
- Type tool_ctx_kitchen_open as ToolContext in Hypothesis test; eliminate
three type: ignore suppressions that resulted from the too-narrow object type
- Document HealthCheck.function_scoped_fixture suppression with inline comment
explaining that the manual state reset in the test body makes reuse safe
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: approved_with_comments
| # Snapshot the caller-supplied values ONLY — NOT _merged_overrides. | ||
| # Storing _merged_overrides would inject stale kitchen_id/diagnostics_log_dir | ||
| # into subsequent load_recipe merges, silently overwriting fresh infra values. | ||
| tool_ctx.session_serve_overrides = dict(overrides) if overrides else {} |
There was a problem hiding this comment.
[warning] bugs: If a caller explicitly passes kitchen_id or diagnostics_log_dir as ingredient overrides to open_kitchen, those keys are captured in session_serve_overrides (L974). On subsequent get_recipe calls, _build_serve_override_stack places snapshot_baseline above session_overrides in priority, so the stale user-supplied value wins over the freshly-computed ctx.kitchen_id / resolve_log_dir(...). The comment at L971-973 warns against storing _merged_overrides for exactly this reason, but raw user overrides containing infra key names have the same effect. Consider stripping known infra keys (kitchen_id, diagnostics_log_dir) from the captured snapshot, or document that users must not pass these keys as overrides.
| manually reset serve-context state at the start of each example to ensure | ||
| open_kitchen always takes the normal path (not deferred-recall) and the | ||
| snapshot is always set from the current example's overrides. | ||
| """ |
There was a problem hiding this comment.
[warning] tests: The Hypothesis test mutates shared fixture state directly (ctx.session_serve_overrides = None, ctx.session_serve_defer_unresolved = False, ctx.recipe_name = "") without using monkeypatch.setattr or a fixture teardown. If a future Hypothesis shrink or DB replay reorders or re-runs examples, partial state from a prior example that raised mid-test (before reaching the reset at L444-447) will leak into the next example. The manual reset only covers the case where the prior example completed normally. Consider using monkeypatch for all three assignments so pytest guarantees rollback on any exit path.
| ) | ||
| assert ok_result.get("success") is True, f"open_kitchen failed: {ok_result}" | ||
| ok_content = ok_result["content"] | ||
|
|
There was a problem hiding this comment.
[warning] tests: The error guard at L274-279 (try: json.loads; pytest.fail except json.JSONDecodeError: pass) will misfire if the production YAML content happens to also be valid JSON (e.g., a recipe containing only a single YAML mapping that starts with {). In that case pytest.fail fires and the test fails even when get_recipe returned correct content. The assert ok_content == gr_content at L281 already catches any real divergence — the JSON guard is redundant and should be removed, or replaced with a direct error-status field check if get_recipe error responses have a distinguishing field.
| overrides: dict[str, str], | ||
| tool_ctx_kitchen_open: ToolContext, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| tmp_path: object, |
There was a problem hiding this comment.
[warning] slop: The 4-line comment block at L433-436 (explaining why manual state reset is needed) is duplicated verbatim at L450-453 in the same function body. The docstring at L434-438 already explains the rationale. Remove the duplicate comment at L450-453 — the reset code at L454-457 is self-explanatory after reading the docstring once.
Trecek
left a comment
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review: warning-only findings detected. See inline comments — no blocking changes required.
Info findings (outside inline comments):
tests/arch/test_serve_surface_registry.py:48[info/tests]: Theallowedset hard-codestools_fleet_dispatch.pyas an exemption but does not assert the file exists on disk. If the file is removed or renamed, the exemption silently remains. Consider assertingall((TOOLS_DIR / name).exists() for name in allowed).tests/arch/test_serve_surface_registry.py:19[info/tests]:_find_load_and_validate_call_filesusesglob('*.py')(non-recursive). Files nested in subdirectories ofserver/tools/would be silently missed. Userglob('*.py')to be future-proof.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…ent xdist leak Add reset_session_serve_overrides() helper to _serve_helpers.py and call it in both setup and teardown sections of the _reset_mcp_tags autouse fixture so that ctx.session_serve_overrides / session_serve_defer_unresolved cannot bleed between tests on the same xdist worker (mirrors _close_kitchen_handler reset). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Trecek
deleted the
load-recipe-re-serve-corrupts-routing-in-live-kitchen-clone/4208-2
branch
1 participant
Summary
A single entry in
__all__ofsrc/autoskillit/core/types/_type_constants_registries.pyis out of alphabetical order."SERVE_SURFACES"is currently appended as the very last entry in__all__, after"SkillCapabilityDef"and"SKILL_CAPABILITY_REGISTRY". Because SERVE precedes SKILL alphabetically,"SERVE_SURFACES"must appear before"SkillCapabilityDef". The fix is to remove"SERVE_SURFACES"from position 38 and insert it at position 36 (between"ALL_VISIBILITY_TAGS"and"SkillCapabilityDef").No other files require changes. The remediation document explicitly states that the implementation is complete and correct in all other respects.
Closes #4208
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/remediation-20260708-002748-757853/.autoskillit/temp/make-plan/serve_surface_symmetry_alphabetical_fix_plan_2026-07-08_030000.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