feat(moments): actions map onto moments, with in-session corrections (milestone 458 step 2, #4920)
CI & Build / Plugin hooks (push) Successful in 18s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 55s
CI & Build / integration (push) Successful in 1m23s
CI & Build / Python tests (push) Successful in 2m0s
CI & Build / Build & push image (push) Successful in 36s
CI & Build / Plugin hooks (push) Successful in 18s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 55s
CI & Build / integration (push) Successful in 1m23s
CI & Build / Python tests (push) Successful in 2m0s
CI & Build / Build & push image (push) Successful in 36s
moment_actions.resolve(tool, input) names every moment a call reaches and the action that reached it. One call can reach several: kubectl apply is a run, a deliver and a reach outside the workspace. Command tools match by how each segment of the line starts, with a word boundary; other tools by field=value arguments. The MCP server prefix and case are ignored. 56 shipped defaults cover the harness tools, Scribe tools and common command shapes. moment_mappings (migration 0116) holds what an install adds and the defaults it switches off. A removal is a stored row, so an upgrade does not switch the default back on. Per the operator ruling, corrections happen in the session: map_action and unmap_action (write tools) return now_reaches so the fix can be confirmed in the same reply. list_moments now shows each moment's actions on this install. REST mirrors both doors, recorded as human. Backup v21 carries the mappings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,119 @@
|
||||
"""Real-Postgres tests for an install's moment mappings (milestone 458 step 2).
|
||||
|
||||
What these pin is what the in-session correction promises the operator: a
|
||||
mapping made is in force on the next call, a default switched off stays off,
|
||||
switching it back on leaves no residue, one user's corrections are not
|
||||
another's, and asking to remove what nothing maps is refused rather than
|
||||
recorded. Every one of those is a claim about rows, so none of it is mocked.
|
||||
"""
|
||||
import pytest
|
||||
import pytest_asyncio
|
||||
from sqlalchemy import delete, select
|
||||
|
||||
from scribe.models import async_session
|
||||
from scribe.models.moment_mapping import MomentMapping
|
||||
from scribe.services import moment_actions as ma
|
||||
from tests.helpers import ensure_user
|
||||
|
||||
pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")]
|
||||
|
||||
OWNER_USERNAME = "moment_mapping_owner"
|
||||
STRANGER_USERNAME = "moment_mapping_stranger"
|
||||
|
||||
SHIP = {"command": "make ship"}
|
||||
CURL = {"command": "curl localhost:8000"}
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def users():
|
||||
async with async_session() as s:
|
||||
owner = await ensure_user(s, OWNER_USERNAME)
|
||||
stranger = await ensure_user(s, STRANGER_USERNAME)
|
||||
await s.commit()
|
||||
ids = (owner.id, stranger.id)
|
||||
# At SETUP: the lane shares one database, so a previous run's rows
|
||||
# are cleared before this one reads anything.
|
||||
await s.execute(delete(MomentMapping).where(MomentMapping.user_id.in_(ids)))
|
||||
await s.commit()
|
||||
return ids
|
||||
|
||||
|
||||
async def _reached(uid, tool, tool_input):
|
||||
return [h["moment"] for h in await ma.moments_for(uid, tool, tool_input)]
|
||||
|
||||
|
||||
async def _rows(uid):
|
||||
async with async_session() as s:
|
||||
return (await s.execute(
|
||||
select(MomentMapping).where(MomentMapping.user_id == uid)
|
||||
)).scalars().all()
|
||||
|
||||
|
||||
async def test_a_mapping_is_in_force_on_the_next_call(users):
|
||||
uid, _ = users
|
||||
assert "work.deliver" not in await _reached(uid, "Bash", SHIP)
|
||||
|
||||
out = await ma.map_action(uid, "Bash", "make ship", "work.deliver",
|
||||
reason="this install ships with make")
|
||||
assert out["change"] == "mapped"
|
||||
assert "work.deliver" in [h["moment"] for h in out["now_reaches"]]
|
||||
assert "work.deliver" in await _reached(uid, "Bash", SHIP)
|
||||
|
||||
|
||||
async def test_mapping_twice_is_one_row(users):
|
||||
uid, _ = users
|
||||
await ma.map_action(uid, "Bash", "make ship", "work.deliver")
|
||||
again = await ma.map_action(uid, "Bash", "make ship", "work.deliver", reason="why")
|
||||
assert again["change"].startswith("already mapped")
|
||||
rows = await _rows(uid)
|
||||
assert len(rows) == 1 and rows[0].reason == "why"
|
||||
|
||||
|
||||
async def test_one_users_corrections_are_not_anothers(users):
|
||||
uid, other = users
|
||||
await ma.map_action(uid, "Bash", "make ship", "work.deliver")
|
||||
assert "work.deliver" not in await _reached(other, "Bash", SHIP)
|
||||
|
||||
|
||||
async def test_unmapping_an_installs_mapping_deletes_it(users):
|
||||
uid, _ = users
|
||||
await ma.map_action(uid, "Bash", "make ship", "work.deliver")
|
||||
out = await ma.unmap_action(uid, "Bash", "make ship", "work.deliver")
|
||||
assert out["change"] == "removed this install's mapping"
|
||||
assert await _rows(uid) == []
|
||||
assert "work.deliver" not in await _reached(uid, "Bash", SHIP)
|
||||
|
||||
|
||||
async def test_a_default_switched_off_stays_off_and_switches_back_cleanly(users):
|
||||
uid, _ = users
|
||||
assert "env.reach" in await _reached(uid, "Bash", CURL)
|
||||
|
||||
off = await ma.unmap_action(uid, "Bash", "curl", "env.reach",
|
||||
reason="curl here only hits the local dev server")
|
||||
assert off["change"] == "switched off the shipped default"
|
||||
assert await _reached(uid, "Bash", CURL) == ["work.run"]
|
||||
[row] = await _rows(uid)
|
||||
assert row.effect == ma.REMOVE
|
||||
|
||||
by_moment = await ma.actions_by_moment(uid)
|
||||
assert [r["match"] for r in by_moment["removed_defaults"]] == ["curl"]
|
||||
assert {"tool": "Bash", "match": "curl", "via": ma.DEFAULT} not in by_moment["actions"]["env.reach"]
|
||||
|
||||
on = await ma.map_action(uid, "Bash", "curl", "env.reach")
|
||||
assert on["change"] == "restored the shipped default"
|
||||
assert await _rows(uid) == []
|
||||
assert "env.reach" in await _reached(uid, "Bash", CURL)
|
||||
|
||||
|
||||
async def test_mapping_a_default_already_in_force_writes_nothing(users):
|
||||
uid, _ = users
|
||||
out = await ma.map_action(uid, "update_task", "status=done", "work.finish")
|
||||
assert out["change"].startswith("already a shipped default")
|
||||
assert await _rows(uid) == []
|
||||
|
||||
|
||||
async def test_removing_what_nothing_maps_is_refused(users):
|
||||
uid, _ = users
|
||||
with pytest.raises(ValueError, match="nothing to remove"):
|
||||
await ma.unmap_action(uid, "Bash", "make ship", "work.deliver")
|
||||
assert await _rows(uid) == []
|
||||
@@ -0,0 +1,181 @@
|
||||
"""Which actions reach which moment (milestone 458 step 2) — the pure half.
|
||||
|
||||
Matching and resolution take no database: the defaults are code and an
|
||||
install's mappings arrive as rows, so every case below hands `resolve` the
|
||||
rows it needs. The writes that store those rows are exercised against real
|
||||
Postgres in `test_integration_moment_mappings.py`.
|
||||
"""
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from scribe.services import moment_actions as ma
|
||||
from scribe.services import moments
|
||||
|
||||
|
||||
def _row(tool, match, moment, effect=ma.ADD):
|
||||
return SimpleNamespace(tool=tool, match=match, moment=moment, effect=effect)
|
||||
|
||||
|
||||
def _reached(tool, tool_input=None, mappings=()):
|
||||
return [h["moment"] for h in ma.resolve(tool, tool_input, mappings)]
|
||||
|
||||
|
||||
# ── the defaults ─────────────────────────────────────────────────────────
|
||||
|
||||
def test_every_default_reaches_a_real_moment():
|
||||
"""A default naming a moment that is not in the catalog would never be
|
||||
mounted on — the typo require_moment refuses at the door."""
|
||||
bad = [a for a in ma.DEFAULT_ACTIONS if not moments.is_moment(a.moment)]
|
||||
assert not bad, bad
|
||||
|
||||
|
||||
def test_no_default_is_listed_twice():
|
||||
keys = [(ma.tool_key(a.tool), a.match, a.moment) for a in ma.DEFAULT_ACTIONS]
|
||||
assert len(keys) == len(set(keys))
|
||||
|
||||
|
||||
def test_every_default_is_a_mapping_the_door_would_accept():
|
||||
"""The defaults pass the same validation an install's mapping does, so a
|
||||
default and an install's copy of it can never disagree about its shape."""
|
||||
for a in ma.DEFAULT_ACTIONS:
|
||||
assert ma._clean(a.tool, a.match, a.moment) == (a.tool, a.match, a.moment)
|
||||
|
||||
|
||||
# ── matching ─────────────────────────────────────────────────────────────
|
||||
|
||||
@pytest.mark.parametrize("command", [
|
||||
"git push origin dev",
|
||||
"git push",
|
||||
"cd app && git push",
|
||||
"make lint; git push --tags",
|
||||
"GIT_TRACE=1 git push",
|
||||
" git push ",
|
||||
])
|
||||
def test_a_command_prefix_matches_any_segment_of_the_line(command):
|
||||
assert "work.deliver" in _reached("Bash", {"command": command})
|
||||
|
||||
|
||||
@pytest.mark.parametrize("command", ["git pushd", "echo git push", "git status"])
|
||||
def test_a_command_prefix_needs_a_word_boundary_and_the_head_of_a_segment(command):
|
||||
assert "work.deliver" not in _reached("Bash", {"command": command})
|
||||
|
||||
|
||||
def test_one_action_reaches_every_moment_it_is():
|
||||
"""`kubectl apply` is a run, a deliver and a reach outside the workspace."""
|
||||
assert _reached("Bash", {"command": "kubectl apply -f deploy.yaml"}) == [
|
||||
"work.run", "work.deliver", "env.reach",
|
||||
]
|
||||
|
||||
|
||||
def test_the_most_specific_action_is_the_one_named():
|
||||
hits = ma.resolve("Bash", {"command": "git push"})
|
||||
deliver = next(h for h in hits if h["moment"] == "work.deliver")
|
||||
assert deliver["match"] == "git push"
|
||||
run = next(h for h in hits if h["moment"] == "work.run")
|
||||
assert run["match"] == ""
|
||||
|
||||
|
||||
def test_an_ordinary_command_is_only_a_run():
|
||||
assert _reached("Bash", {"command": "ls -la"}) == ["work.run"]
|
||||
|
||||
|
||||
def test_an_unknown_tool_reaches_nothing():
|
||||
assert _reached("SomeOtherTool", {"x": 1}) == []
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name", [
|
||||
"mcp__plugin_scribe_scribe__update_task", "mcp__scribe__update_task",
|
||||
"update_task", "Update_Task",
|
||||
])
|
||||
def test_the_server_prefix_and_case_do_not_matter(name):
|
||||
assert _reached(name, {"task_id": 1, "status": "done"}) == ["work.finish"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("status,expected", [
|
||||
("done", ["work.finish"]),
|
||||
("cancelled", ["work.finish"]),
|
||||
("in_progress", ["work.start"]),
|
||||
("todo", []),
|
||||
("", []),
|
||||
])
|
||||
def test_arguments_select_the_moment(status, expected):
|
||||
assert _reached("update_task", {"task_id": 1, "status": status}) == expected
|
||||
|
||||
|
||||
def test_a_whole_tool_mapping_ignores_its_arguments():
|
||||
assert _reached("Edit", {"file_path": "x", "old_string": "a"}) == ["work.change"]
|
||||
|
||||
|
||||
def test_loading_a_procedure_reaches_its_own_moment():
|
||||
assert _reached("Skill", {"skill": "scribe:writing-plans"}) == [
|
||||
"skill.scribe:writing-plans",
|
||||
]
|
||||
|
||||
|
||||
def test_a_procedure_name_that_is_not_a_moment_reaches_nothing():
|
||||
assert _reached("Skill", {"skill": "two words"}) == []
|
||||
assert _reached("Skill", {}) == []
|
||||
|
||||
|
||||
# ── an install's own ─────────────────────────────────────────────────────
|
||||
|
||||
def test_an_install_mapping_adds_a_moment():
|
||||
rows = [_row("Bash", "make ship", "work.deliver")]
|
||||
assert "work.deliver" in _reached("Bash", {"command": "make ship"}, rows)
|
||||
assert "work.deliver" not in _reached("Bash", {"command": "make ship"})
|
||||
|
||||
|
||||
def test_removing_a_default_removes_only_that_default():
|
||||
"""`curl` stops reaching env.reach here; it is still a run, and `ssh`
|
||||
still reaches out — a removal overrides nothing it did not name."""
|
||||
rows = [_row("Bash", "curl", "env.reach", ma.REMOVE)]
|
||||
assert _reached("Bash", {"command": "curl localhost:8000"}, rows) == ["work.run"]
|
||||
assert "env.reach" in _reached("Bash", {"command": "ssh box"}, rows)
|
||||
|
||||
|
||||
def test_a_removal_matches_the_default_whatever_the_tool_is_called():
|
||||
rows = [_row("mcp__x__update_task", "status=done", "work.finish", ma.REMOVE)]
|
||||
assert _reached("update_task", {"status": "done"}, rows) == []
|
||||
|
||||
|
||||
def test_an_install_mapping_is_named_as_the_installs():
|
||||
rows = [_row("Bash", "make ship", "work.deliver")]
|
||||
hit = next(h for h in ma.resolve("Bash", {"command": "make ship"}, rows)
|
||||
if h["moment"] == "work.deliver")
|
||||
assert (hit["via"], hit["match"]) == (ma.INSTALL, "make ship")
|
||||
|
||||
|
||||
# ── validation ───────────────────────────────────────────────────────────
|
||||
|
||||
def test_a_prefix_on_a_tool_that_runs_no_command_is_refused():
|
||||
"""It would be stored and never match — a mapping that looks made and is
|
||||
not, which is the misfire this table exists to fix."""
|
||||
with pytest.raises(ValueError, match="field=value"):
|
||||
ma._clean("update_task", "done", "work.finish")
|
||||
|
||||
|
||||
def test_an_unknown_moment_is_refused():
|
||||
with pytest.raises(ValueError, match="unknown moment"):
|
||||
ma._clean("Bash", "make ship", "work.ship")
|
||||
|
||||
|
||||
def test_a_mapping_is_normalised_before_it_is_stored():
|
||||
assert ma._clean("mcp__x__update_task", " status=done ", " Work.Finish") == (
|
||||
"update_task", "status=done", "work.finish",
|
||||
)
|
||||
assert ma._clean("Bash", "make ship", "work.deliver")[1] == "make ship"
|
||||
|
||||
|
||||
def test_a_tool_is_required():
|
||||
with pytest.raises(ValueError, match="tool is required"):
|
||||
ma._clean(" ", "", "work.run")
|
||||
|
||||
|
||||
# ── failing open ─────────────────────────────────────────────────────────
|
||||
|
||||
async def test_an_unreadable_mapping_table_costs_the_corrections_not_the_moments():
|
||||
with patch.object(ma, "list_mappings", AsyncMock(side_effect=RuntimeError("db down"))):
|
||||
hits = await ma.moments_for(7, "Bash", {"command": "git push"})
|
||||
assert [h["moment"] for h in hits] == ["work.run", "work.deliver"]
|
||||
+22
-7
@@ -93,27 +93,42 @@ def test_the_catalog_payload_carries_every_moment_and_the_family():
|
||||
assert data["families"][0]["prefix"] == moments.SKILL_PREFIX
|
||||
|
||||
|
||||
async def test_the_tool_returns_the_service_catalog():
|
||||
async def test_the_tool_returns_the_catalog_with_this_installs_actions():
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
from scribe.mcp.tools import moments as tool
|
||||
|
||||
assert await tool.list_moments() == moments.catalog()
|
||||
by_moment = {"actions": {"work.change": []}, "removed_defaults": []}
|
||||
with patch.object(tool, "current_user_id", lambda: 7), \
|
||||
patch.object(tool.actions_svc, "actions_by_moment",
|
||||
AsyncMock(return_value=by_moment)) as read:
|
||||
out = await tool.list_moments()
|
||||
read.assert_awaited_once_with(7)
|
||||
assert out["moments"] == moments.catalog()["moments"]
|
||||
assert out["actions"] == by_moment["actions"]
|
||||
assert out["removed_defaults"] == []
|
||||
|
||||
|
||||
def test_the_tool_is_registered_and_read_only():
|
||||
from scribe.mcp.server import _READ_ONLY_TOOLS
|
||||
def test_the_tools_are_registered_and_classified():
|
||||
from scribe.mcp.server import _READ_ONLY_TOOLS, _WRITE_TOOLS
|
||||
from scribe.mcp.tools import moments as tool
|
||||
|
||||
mcp = FakeMCP()
|
||||
tool.register(mcp)
|
||||
assert mcp.names == ["list_moments"]
|
||||
assert mcp.names == ["list_moments", "map_action", "unmap_action"]
|
||||
assert "list_moments" in _READ_ONLY_TOOLS
|
||||
assert {"map_action", "unmap_action"} <= _WRITE_TOOLS
|
||||
|
||||
|
||||
def test_both_doors_read_one_catalog():
|
||||
"""Rule 33 parity: the tool and the route call the same service, so the
|
||||
session and the Settings view cannot name different moments."""
|
||||
"""Rule 33 parity: the tool and the routes call the same services, so the
|
||||
session and the Settings view cannot name different moments or disagree
|
||||
about what a mapping does."""
|
||||
from scribe.mcp.tools import moments as tool
|
||||
from scribe.routes import retrieval as routes
|
||||
from scribe.services import moment_actions
|
||||
|
||||
assert tool.moments_svc is moments
|
||||
assert routes.moments_svc is moments
|
||||
assert tool.actions_svc is moment_actions
|
||||
assert routes.moment_actions_svc is moment_actions
|
||||
|
||||
@@ -43,6 +43,7 @@ def test_every_endpoint_is_reachable_on_the_app():
|
||||
"/api/retrieval/surfaces/<surface>",
|
||||
"/api/retrieval/tuning-history",
|
||||
"/api/retrieval/moments",
|
||||
"/api/retrieval/moments/mappings",
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -27,7 +27,7 @@ def test_backup_version_is_current():
|
||||
|
||||
(Named for the number it asserted until v10, which is exactly the drift a
|
||||
name-carrying-a-value invites; it now says what it checks.)"""
|
||||
assert backup.BACKUP_VERSION == 20
|
||||
assert backup.BACKUP_VERSION == 21
|
||||
|
||||
|
||||
def _exportable_note(**over):
|
||||
@@ -141,6 +141,7 @@ def _column_guard_targets():
|
||||
from scribe.models.rule_usage import RuleUsageEvent
|
||||
from scribe.models.system_usage import SystemUsageEvent
|
||||
from scribe.models.retrieval_tuning import RetrievalTuningEvent
|
||||
from scribe.models.moment_mapping import MomentMapping
|
||||
from scribe.models.note_version import NoteVersion
|
||||
from scribe.models.rule_version import RuleVersion
|
||||
from scribe.models.project import Project
|
||||
@@ -177,6 +178,7 @@ def _column_guard_targets():
|
||||
"retrieval_tuning_events": (
|
||||
RetrievalTuningEvent, backup._retrieval_tuning_event_rows,
|
||||
),
|
||||
"moment_mappings": (MomentMapping, backup._moment_mapping_rows),
|
||||
"design_systems": (DesignSystem, backup._design_system_rows),
|
||||
"design_tokens": (DesignToken, backup._design_token_rows),
|
||||
"repo_bindings": (RepoBinding, backup._repo_binding_rows),
|
||||
@@ -294,6 +296,7 @@ def _import_guard_targets():
|
||||
"rule_usage_events": backup._build_rule_usage_event,
|
||||
"system_usage_events": backup._build_system_usage_event,
|
||||
"retrieval_tuning_events": backup._build_retrieval_tuning_event,
|
||||
"moment_mappings": backup._build_moment_mapping,
|
||||
"design_systems": backup._build_design_system,
|
||||
"design_tokens": backup._build_design_token,
|
||||
"repo_bindings": backup._build_repo_binding,
|
||||
|
||||
Reference in New Issue
Block a user