fix(skills): address /review findings 2/3/4 from PR 555

Three independent fixes flagged by Copilot's review on PR #555:

2. ``update`` auto_approve self-escalation warning false-positive
   (turnstone/core/session.py:_prepare_skills_update)
   - The warning was computed against ``existing_auto_approve or
     proposed_auto_approve`` — meaning an update that explicitly
     turned auto_approve OFF still triggered the warning because the
     existing row had it ON.  Now computes against the *final state*
     (``updates["auto_approve"]`` if present, else
     ``existing.get("auto_approve")``) combined with the final
     ``allowed_tools`` value.  False-positives gone; the inverse case
     (existing auto_approve=False, update turns it ON without
     touching allowed_tools) now correctly fires the warning against
     the inherited allowlist.

3. ``temperature`` validator silent-coerce → explicit error
   (turnstone/core/skill_field_validation.py:parse_skill_session_config)
   - Non-numeric temperature input silently coerced to ``None``,
     unlike ``max_tokens`` / ``token_budget`` which return an error.
     Numeric-field consistency: temperature now errors on
     unparseable input with "temperature must be a number between 0
     and 2".  Range check unchanged; blank / None still → None.

4. Version-snapshot uses max+1, not count+1
   (turnstone/core/session.py:_exec_skills_update)
   - ``count_skill_versions + 1`` re-uses version numbers when any
     row has been deleted via the existing
     ``storage.delete_skill_versions`` method, and the schema has no
     ``(skill_id, version)`` unique constraint to catch the
     collision.  Switched to ``max(list_skill_versions)`` + 1,
     matching the ``storage.unlock_skill`` pattern.  A storage-side
     atomic allocator is the right architectural fix and is tracked
     for a future PR.

Tests cover both the false-positive and inverse-positive auto_approve
cases, the new temperature error path, and the version-numbering edge
case where prior versions have been deleted (max diverges from count).

tests/test_skills_tool.py

@@ -739,6 +739,26 @@ class TestExecSkillsCreate:
             )
         assert "'content' is required" in item.get("error", "")
 
+    def test_create_invalid_temperature_errors(self) -> None:
+        """Non-numeric temperature input now returns an explicit error
+        rather than silently coercing to None.  Matches the max_tokens /
+        token_budget shape — every numeric field on the validator errors
+        loudly on bad input, no silent coerce."""
+        session = _make_session()
+        with patch("turnstone.core.auth.user_has_permission", return_value=True):
+            item = session._prepare_skills(
+                "c",
+                {
+                    "action": "create",
+                    "name": "x",
+                    "content": "b",
+                    "description": "d",
+                    "temperature": "not-a-number",
+                },
+            )
+        err = item.get("error", "")
+        assert "temperature must be a number" in err, err
+
     def test_create_invalid_kind_errors(self) -> None:
         """SkillKind ValueError branch — model passes unknown kind, gets
         explicit listing of valid values rather than a stack trace."""
@@ -812,6 +832,54 @@ class TestExecSkillsUpdate:
             "readonly": False,
         }
 
+    def test_update_auto_approve_warning_uses_final_state(self) -> None:
+        """The auto_approve+allowed_tools self-escalation warning must fire
+        against the *final* state (post-update), not the existing row alone.
+        Previously: ``existing_auto_approve=True`` triggered the warning
+        even when the update explicitly turned auto_approve OFF.
+        """
+        # Case A: existing auto_approve=True, update turns it OFF.
+        # Allowed_tools change present.  Warning must NOT fire (final
+        # state is auto_approve=False).
+        row = self._existing_row()
+        row["auto_approve"] = True
+        session = _make_session()
+        storage = MagicMock()
+        storage.get_prompt_template_by_name.return_value = row
+        with (
+            patch("turnstone.core.auth.user_has_permission", return_value=True),
+            patch("turnstone.core.storage._registry.get_storage", return_value=storage),
+        ):
+            item = session._prepare_skills(
+                "c",
+                {
+                    "action": "update",
+                    "name": "existing",
+                    "auto_approve": False,
+                    "allowed_tools": ["bash"],
+                },
+            )
+        assert "WARNING: auto_approve" not in item["preview"], item["preview"]
+
+        # Case B: existing auto_approve=False, update turns it ON.
+        # Allowed_tools inherited (not in updates).  Warning MUST fire
+        # (final state is auto_approve=True with inherited allowlist).
+        row_b = self._existing_row()
+        row_b["auto_approve"] = False
+        row_b["allowed_tools"] = '["bash"]'
+        storage_b = MagicMock()
+        storage_b.get_prompt_template_by_name.return_value = row_b
+        session_b = _make_session()
+        with (
+            patch("turnstone.core.auth.user_has_permission", return_value=True),
+            patch("turnstone.core.storage._registry.get_storage", return_value=storage_b),
+        ):
+            item_b = session_b._prepare_skills(
+                "c",
+                {"action": "update", "name": "existing", "auto_approve": True},
+            )
+        assert "WARNING: auto_approve" in item_b["preview"], item_b["preview"]
+
     def test_update_includes_projected_risk_on_preview(self) -> None:
         session = _make_session()
         storage = MagicMock()
@@ -854,11 +922,22 @@ class TestExecSkillsUpdate:
         assert "runtime config" in item.get("error", "")
 
     def test_update_snapshots_to_skill_versions(self) -> None:
+        """Snapshot version uses max(existing version) + 1 — NOT count+1.
+        Count-based numbering re-uses version numbers after a
+        ``delete_skill_versions`` call (which is a real storage method),
+        leading to ``(skill_id, version)`` collisions on the next insert.
+        Matches the ``storage.unlock_skill`` pattern.
+        """
         session = _make_session()
         storage = MagicMock()
         storage.get_prompt_template_by_name.return_value = self._existing_row()
         storage.get_prompt_template.return_value = self._existing_row()
-        storage.count_skill_versions.return_value = 2
+        # Simulate a row whose history had v1, v2, v3 — then v1 + v2 were
+        # deleted (e.g. retention policy).  Count is 1; max is 3.  Next
+        # version must be 4, not 2.
+        storage.list_skill_versions.return_value = [
+            {"version": 3, "changed_by": "admin", "created": "..."},
+        ]
         with (
             patch("turnstone.core.auth.user_has_permission", return_value=True),
             patch("turnstone.core.storage._registry.get_storage", return_value=storage),
@@ -867,14 +946,29 @@ class TestExecSkillsUpdate:
                 "c", {"action": "update", "name": "existing", "description": "new"}
             )
             session._exec_skills(item)
-        # Pre-update snapshot uses count+1 (the linear-version convention)
-        # rather than len(list)+1, avoiding the (skill_id, version)
-        # collision risk on concurrent updates noted in the boundary spike.
         storage.create_skill_version.assert_called_once()
         kwargs = storage.create_skill_version.call_args.kwargs
-        assert kwargs["version"] == 3
+        assert kwargs["version"] == 4, "max+1 must use max(existing version), not count+1"
         assert kwargs["changed_by"] == session._user_id
 
+    def test_update_snapshots_starts_at_v1_on_empty_history(self) -> None:
+        """Edge case: no existing versions → next version is 1."""
+        session = _make_session()
+        storage = MagicMock()
+        storage.get_prompt_template_by_name.return_value = self._existing_row()
+        storage.get_prompt_template.return_value = self._existing_row()
+        storage.list_skill_versions.return_value = []
+        with (
+            patch("turnstone.core.auth.user_has_permission", return_value=True),
+            patch("turnstone.core.storage._registry.get_storage", return_value=storage),
+        ):
+            item = session._prepare_skills(
+                "c", {"action": "update", "name": "existing", "description": "new"}
+            )
+            session._exec_skills(item)
+        kwargs = storage.create_skill_version.call_args.kwargs
+        assert kwargs["version"] == 1
+
 
 class TestExecSkillsToggle:
     def test_disable_audits_with_actor_source(self) -> None:

turnstone/core/session.py

@@ -8595,17 +8595,20 @@ class ChatSession:
             else:
                 vstr = str(v)
                 preview_lines.append(f"    {k}: {vstr[:120]}")
-        # Self-escalation warning: if the update turns on auto_approve
-        # OR expands allowed_tools while a pre-existing auto_approve is
-        # set, spell out the operational consequence on the approval
-        # card.  Same shape as the create-side warning.
-        proposed_auto_approve = bool(updates.get("auto_approve"))
-        existing_auto_approve = bool(existing.get("auto_approve"))
-        at_changes = "allowed_tools" in updates and updates["allowed_tools"] not in (
-            "",
-            "[]",
+        # Self-escalation warning: if the *final* state has both
+        # auto_approve=True AND non-empty allowed_tools, spell out the
+        # operational consequence on the approval card.  Compute against
+        # the resolved final state — not just the existing row — so an
+        # update that explicitly turns auto_approve OFF doesn't false-
+        # positive the warning, and an update that turns it ON without
+        # touching allowed_tools still fires it against the inherited
+        # allowlist.
+        final_auto_approve = bool(
+            updates["auto_approve"] if "auto_approve" in updates else existing.get("auto_approve")
         )
-        if (proposed_auto_approve or existing_auto_approve) and at_changes:
+        final_allowed_tools = updates.get("allowed_tools", existing.get("allowed_tools", "[]"))
+        final_has_allowed_tools = bool(final_allowed_tools) and final_allowed_tools != "[]"
+        if final_auto_approve and final_has_allowed_tools:
             preview_lines.append(
                 "    WARNING: auto_approve + allowed_tools means the listed "
                 "tools auto-fire when this skill is loaded"
@@ -8681,10 +8684,18 @@ class ChatSession:
             item["updates"] = filtered
             item["readonly"] = True
         # Snapshot existing row to skill_versions for rollback.  Uses
-        # count_skill_versions + 1 so concurrent updates don't collide on
-        # the (skill_id, version) unique key.
+        # max(version) + 1 (via list_skill_versions, which returns rows
+        # ordered by version DESC so [0] is the max) instead of
+        # count + 1 — ``count`` re-uses numbers when versions have been
+        # deleted via storage.delete_skill_versions, and the schema has
+        # no (skill_id, version) unique constraint to catch collisions.
+        # Matches the storage-level pattern in storage.unlock_skill.
+        # A storage-side allocator (atomic max+1 with a unique index)
+        # is the right architectural fix and is tracked separately.
         try:
-            next_version = storage.count_skill_versions(template_id) + 1
+            existing_versions = storage.list_skill_versions(template_id)
+            current_max = max((int(v.get("version") or 0) for v in existing_versions), default=0)
+            next_version = current_max + 1
             storage.create_skill_version(
                 skill_id=template_id,
                 version=next_version,

turnstone/core/skill_field_validation.py

@@ -50,7 +50,10 @@ def parse_skill_session_config(body: dict[str, Any]) -> tuple[dict[str, Any], st
 
     Field rules:
 
-    - ``temperature``: float in [0.0, 2.0] or None / "" → None
+    - ``temperature``: float in [0.0, 2.0] or None / "" → None.
+      Non-numeric input (string that doesn't parse, dict, list) errors
+      out — matches ``max_tokens`` / ``token_budget`` for numeric-field
+      consistency.
     - ``max_tokens``: int >= 1 or None / "" → None
     - ``token_budget``: int >= 0 (defaults to 0 if missing-but-empty)
     - ``agent_max_turns``: int >= 1 or None / "" → None
@@ -70,16 +73,16 @@ def parse_skill_session_config(body: dict[str, Any]) -> tuple[dict[str, Any], st
 
     if "temperature" in body:
         temp = body["temperature"]
-        if temp is not None and temp != "":
+        if temp is None or temp == "":
+            fields["temperature"] = None
+        else:
             try:
                 temp = float(temp)
-                if not (0.0 <= temp <= 2.0):
-                    return {}, "temperature must be between 0 and 2"
-                fields["temperature"] = temp
             except (ValueError, TypeError):
-                fields["temperature"] = None
-        else:
-            fields["temperature"] = None
+                return {}, "temperature must be a number between 0 and 2"
+            if not (0.0 <= temp <= 2.0):
+                return {}, "temperature must be between 0 and 2"
+            fields["temperature"] = temp
 
     if "token_budget" in body:
         try: