From ba34013bbbf15b7fe87447243421ac2a6c6cbda7 Mon Sep 17 00:00:00 2001 From: Tomas McMonigal <74835766+sudoTomas@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:58:12 +0000 Subject: [PATCH] feat(slop): slop routing policy as a settings section What embry0 did with a code job or a finding judged slop was code. Every code job judged slop ran slop-cleanup, seeded again at each start so deleting it did not stick. The kinds a verdict could name, what the classifiers and the reviewer were told, the heading of a slop finding's ticket and the meter levels triage leaned at and dungeons appeared from were constants, and the lean level was never read. Add a slop_policy section: enabled, route_template, min_score_to_route, kinds, classifier_guide, review_guide, ticket_heading, triage_lean_min_level and dungeon_min_level. Each default is the old constant, so the default triage prompt is the old one to the byte. An organization may set every key, a repository only the three that route its own jobs: the guides go into system prompts, which a key with a repository allowlist must not write. A job reads the section when it starts, the watcher its stack's on each analysis and filing, the world each repository's dungeon level when it draws. Triage's paragraph is built from the policy: the heading it weighs, what a verdict does to the route, the route by name unless it is the built-in cleanup, and the lean, named with the meter's own level names. Its slots are filled in one pass, so an operator's text is never read for a marker. plan_route sends a code job judged slop to the policy's template when routing is on and the score reaches the threshold. A job pinned to the full pipeline keeps review and QA: it takes the slop route only when that route has both. A template that is gone, invalid or unfit is logged and the job takes its kind's default, and never fails. A write that changes the template a document's readers route to, by naming another or by taking the key or the section out so they fall back to what they inherit or the default, must leave one that records code jobs and has a coding step; a section's new effective_check sees the default fallback, which a field validator never does. Dry runs and undo are checked the same way. Only a write that may change slop_policy reads the templates, FOR SHARE in its transaction. Deleting, renaming or unfitting a template a document routes to answers 409 naming the documents; the change locks its row FOR UPDATE before it reads them, so neither side races the other. The built-in templates are still seeded at every start: routing no longer depends on whether the template exists. No variable fed these keys, so nothing is imported. A cleanup's experience stays the progression pack's outcomes.cleaned (#176). METER_DUNGEON_LEVEL moves into the policy beside the lean level: both are where embry0 acts on a meter reading, not how a score is computed. Fixes #169 --- docs/api.md | 5 +- docs/configuration.md | 17 + docs/examples/settings.yaml | 86 ++++ docs/glossary.md | 2 +- docs/slop-meter.md | 4 +- docs/slop-routing.md | 102 +++++ embry0/api/app.py | 16 + embry0/api/v1/pipeline_templates.py | 22 +- embry0/api/v1/world.py | 19 +- embry0/orchestration/nodes/triage.py | 105 ++++- embry0/services/job_settings.py | 7 +- embry0/services/proposals.py | 4 +- embry0/services/slop.py | 135 +++--- embry0/services/slop_meter/context.py | 22 + embry0/services/slop_routing.py | 116 +++++ embry0/services/watcher.py | 64 ++- embry0/settings/document.py | 80 +++- embry0/settings/example.py | 10 +- embry0/settings/registry.py | 15 +- embry0/settings/sections/__init__.py | 2 + embry0/settings/sections/slop_policy.py | 332 ++++++++++++++ embry0/settings/store.py | 88 +++- embry0/settings/write_context.py | 69 ++- .../repositories/pipeline_templates.py | 35 +- .../repositories/settings_documents.py | 7 +- embry0/workflows/issue_to_pr/nodes/_review.py | 11 +- embry0/workflows/issue_to_pr/nodes/_triage.py | 13 +- embry0/workflows/issue_to_pr/plan_route.py | 82 +++- embry0/workflows/row_behaviour.py | 45 ++ .../unit/agents/test_triage_may_add_skills.py | 5 + .../api/test_slop_policy_settings_live.py | 426 ++++++++++++++++++ tests/unit/api/test_watcher_wiring.py | 2 + .../unit/api/v1/test_behaviour_writes_api.py | 5 +- .../api/v1/test_pipeline_templates_api.py | 4 +- tests/unit/api/v1/test_world_slop_policy.py | 67 +++ .../nodes/test_triage_model_guidance.py | 4 + .../orchestration/nodes/test_triage_node.py | 8 +- .../nodes/test_triage_slop_policy.py | 169 +++++++ tests/unit/services/test_job_settings.py | 3 + tests/unit/services/test_slop.py | 34 +- tests/unit/services/test_slop_framing.py | 4 + .../unit/services/test_slop_policy_readers.py | 135 ++++++ tests/unit/services/test_watcher.py | 2 +- .../unit/services/test_watcher_slop_policy.py | 122 +++++ tests/unit/settings/test_example.py | 5 +- .../unit/settings/test_slop_policy_section.py | 306 +++++++++++++ .../unit/workflows/test_policy_efficiency.py | 4 + .../unit/workflows/test_slop_route_policy.py | 187 ++++++++ 48 files changed, 2787 insertions(+), 220 deletions(-) create mode 100644 docs/slop-routing.md create mode 100644 embry0/services/slop_routing.py create mode 100644 embry0/settings/sections/slop_policy.py create mode 100644 tests/unit/api/test_slop_policy_settings_live.py create mode 100644 tests/unit/api/v1/test_world_slop_policy.py create mode 100644 tests/unit/orchestration/nodes/test_triage_slop_policy.py create mode 100644 tests/unit/services/test_slop_policy_readers.py create mode 100644 tests/unit/services/test_watcher_slop_policy.py create mode 100644 tests/unit/settings/test_slop_policy_section.py create mode 100644 tests/unit/workflows/test_slop_route_policy.py diff --git a/docs/api.md b/docs/api.md index 19262db7..ff3ce2da 100644 --- a/docs/api.md +++ b/docs/api.md @@ -487,11 +487,12 @@ A scanner's map of a codebase or an organization, marked and sent to agents ([ma ## Pipeline templates -The saved pipeline graphs the editor and the Pipelines page work on, under `/api/v1/pipelines/templates`: list, read, create, update, delete and duplicate. Every authenticated principal may read and write them; the two writes with answers worth knowing: +The saved pipeline graphs the editor and the Pipelines page work on, under `/api/v1/pipelines/templates`: list, read, create, update, delete and duplicate. Every authenticated principal may read and write them; the writes with answers worth knowing: | Endpoint | Answer | |---|---| -| `PUT /api/v1/pipelines/templates/{id}` | Updates the fields sent. `default_for_kind` set to a kind makes the template that kind's default route; an explicit `null` clears it. 409 when another template already holds the kind (clear it first), or when the new name is taken | +| `PUT /api/v1/pipelines/templates/{id}` | Updates the fields sent. `default_for_kind` set to a kind makes the template that kind's default route; an explicit `null` clears it. 409 when another template already holds the kind (clear it first), or when the new name is taken. 409 too for a rename, or a change that makes it stop taking a code job, of a template a settings document's `slop_policy.route_template` routes slop jobs to ([slop routing](slop-routing.md#the-route-template)); the answer names those documents | +| `DELETE /api/v1/pipelines/templates/{id}` | Deletes the template. 409, naming the documents, while a settings document's `slop_policy.route_template` routes slop jobs to it, `slop-cleanup` by default ([slop routing](slop-routing.md#the-route-template)) | | `POST /api/v1/pipelines/templates/{id}/duplicate` | `{"name": "..."}` copies the graph, the sandbox profile and what the row says of its jobs (below) under the new name and answers 201; the copy is no kind's default. 409 when a template of that name already exists | **What a template's row says of its jobs.** Five columns (#167) decide how embry0 treats a job of the template, where it once went by the built-in templates' names. `POST` accepts them, `PUT` changes the ones sent, and every read, the list included, returns them; on the Pipelines page a template's menu edits them under *Behaviour…*: diff --git a/docs/configuration.md b/docs/configuration.md index 9ee78522..c6d20273 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -319,6 +319,7 @@ asks: - The salvage policy (`salvage`, until now fixed in the code as `wip: unfinished — stopped ()`): the subject of the commit that keeps a stopped coding run's work, as a template naming `{agent}` and `{reason}`, so a repository whose commit rules refuse `wip:` can write what they accept. A subject past 100 characters has only its reason shortened. An organization's or a repository's layer may set it, and a coding job reads it when it starts, as it reads `done_when`. - The scout's policy (`scout`, from `SCOUT_TEMPLATES`, `SCOUT_MAX_IN_FLIGHT`, `SCOUT_REVISIT_HOURS` and `SCOUT_MAX_LEADS_PER_JOB`, plus each job's hard cap, until now fixed at 1 USD, and a daily cap, off by default): the scout reads it every tick, so a change applies within 5 minutes with no restart. Which repositories it reads is `background.scout` (above). - The Slop-O-Meter's scoring policy and pace (`slop_meter`, from `SLOP_METER_INTERVAL_SECONDS`, which is `interval_hours`, or `enabled: false` for `0`, `SLOP_METER_API_CALLS_PER_TICK` and `SLOP_METER_RATE_RESERVE`; everything else was fixed in the meter's code, and its defaults are those values, so a deployment that writes nothing scores exactly as before): what a changed line of each kind of file weighs (`weights`, merged glob by glob), which directories are vendored and what is a test, the hours a commit, a co-author or a comment implies, the lines an hour covers and the weekly cap, who counts as the core team and the small-repository rule, the outlier thresholds and what the judge is told, the five levels' bounds and names, and the owner's own bots and AI agents. An organization's layer may set every key but `enabled` and `interval_hours`, and a repository's what counts as code in its layout. Each organization's `slop_meter_bots` was copied into its layer as `bots` once, each bot once and an entry the section cannot hold left out; `PATCH /api/v1/orgs/{owner}` still takes it until 0.2.0. `bots` and `ai_agents` add to what a layer inherits. A credential's forge budget is the deployment's; an organization's `api_calls_per_tick` and `rate_reserve` only narrow its own share. Every score records the version of the settings it was computed with; every 5 minutes the meter compares it with the version each repository reads now and rescores only those that differ, from what it read, with no fetch and no forge call ([how a change rescores](slop-meter.md#how-a-change-rescores)). Until the first start's import has imported the three variables, the meter reads them as before. The variables left (`SLOP_METER_CATCHUP_SECONDS`, `_DATA_PATH`, `_GIT_TIMEOUT_SECONDS`, `_COMMITS_PER_TICK`, `_EVENT_PAGES`, `_REVIEWS_PER_TICK`, `_LOGINS_PER_TICK`, `_JUDGMENTS_PER_TICK`) are the process's own limits. + - The slop policy (`slop_policy`, until now fixed in the code: every code job judged slop ran `slop-cleanup`, the seven kinds, the classifiers' and the reviewer's words, the ticket heading "Slop, not a bug", and the meter's dungeon level 2 and lean level 4; no variable set them, so nothing is imported, and a deployment that writes nothing routes and prompts exactly as before): whether and where a code job judged slop is routed and from which score, which an organization's or a repository's layer may set, and the kinds, the guides, the heading and the two levels, which only an organization's may. A job reads it for its repository when it starts, the watcher for its stack's repository on every analysis. `route_template` must name a template that takes a code job, and such a template cannot be deleted or renamed while a document routes to it ([slop routing](slop-routing.md)). - The dashboard's game layer and home page (`ui`, new: no variable configured them). The game is on and the guild is the home until a document says otherwise. The dashboard reads the section before it draws its navigation and home page, every minute after that and at once after a save on the Policy tab, and `GET /api/v1/config/ui` and the game's routes read it on every request, so turning the game off or on needs no restart and no reload. The deployment's only: the dashboard knows no viewer's organization to read a layer for. See [turning the game off](world.md#turning-the-game-off). ### Which repositories background features read @@ -578,6 +579,22 @@ How the Slop-O-Meter weighs a changed line and a person's attention, where its l | `bots` | list of text | `[]` | at most 200 entries, each at most 200 characters | organization, added to | | Bots, as commit names, addresses or forge logins matched whole without regard to case: their commits add lines but no attention, and as a co-author they count as an AI agent does. Beyond the ones every deployment knows ([bot] accounts, noreply@ addresses, known coding agents) and embry0's own commit identities. Each document adds to the list it inherits, so a bot the deployment lists counts for every organization; no layer takes one out, and a bot spelled otherwise is the same one. At most 200 in all, counted with what a write inherits. Was the organization's slop_meter_bots. | | `ai_agents` | list of text | `[]` | at most 200 entries, each at most 200 characters | organization, added to | | AI coding agents beyond the known ones, each its commit address up to and including the @ (aider@, any domain) or a whole address (agent@tools.example), matched without regard to case: a commit one authors adds lines but no attention, and one it co-authors counts ai_coauthor_share of its author's hours. Each document adds to the list it inherits, as bots. At most 200 in all, counted with what a write inherits. | +#### `slop_policy` + +What embry0 does with a code job or a finding judged slop: whether and where it is routed, what the classifiers and the reviewer are told, its kinds, its ticket heading and the meter levels it acts on. + +| Key | Type | Default | Allowed | Overridable by | Imported from | Description | +|---|---|---|---|---|---|---| +| `enabled` | true or false | `true` | | organization, repository | | A code job judged slop runs route_template instead of the route triage picked. False: it takes its normal route, the verdict is still recorded on the job, and triage is told so. | +| `route_template` | text | `"slop-cleanup"` | at least 1 characters, at most 100 characters | organization, repository | | The pipeline template a code job judged slop runs. A write that changes it, by naming another or by taking the key out, must leave a template that takes a code job: one whose jobs are recorded as code jobs and whose route has a coding step. A template a document routes to cannot be deleted, renamed or changed to stop taking one. A job pinned to the full pipeline takes it only when its route also has review and QA. A template a person named for the job still wins. | +| `min_score_to_route` | whole number | `0` | ≥ 0, ≤ 100 | organization, repository | | The least slop_score (0 to 100, how much of the trouble is shape) a slop verdict needs to be routed. 0 routes every slop verdict; one below it takes its normal route. | +| `kinds` | list of text | `["duplication", "dead_code", "half_wired", "contradicting_layers", "symptom_patch", "misleading_docs", "off_pattern"]` | at most 20 entries, each at most 40 characters | organization | | The kinds of slop a verdict may name, as lowercase words joined by _. A kind a classifier names that is not here is dropped from its verdict. Describe each in classifier_guide, which is what the classifiers are told. | +| `classifier_guide` | text | `"Tell slop apart from bugs:\n- bug: the behavior differs from what the code, its docs or the ticket intend.\n Fixing it changes behavior.\n- slop: the behavior is what the code says, and the trouble is the code's\n shape. Cleaning it keeps behavior. Kinds: duplication (the same logic in\n more than one place), dead_code (paths, flags or files nothing reaches),\n half_wired (a feature or abstraction started and not finished),\n contradicting_layers (two layers that disagree about one thing),\n symptom_patch (guards, retries, fallbacks or catch-alls stacked on a symptom\n instead of its cause), misleading_docs (comments or docs that say what the\n code does not do), off_pattern (code that ignores the repository's own way).\n- noise: nothing to do.\nA guard, retry or catch-all stacked on a symptom is slop when removing it and\nfixing the shape keeps the outcome. When the code does the wrong thing (keeps\nretrying what can never succeed, drops or duplicates data, answers wrongly),\nit is a bug, even when sloppy code caused it: say bug, and name the slop in\nthe evidence. Do not call a real defect slop to make it look smaller.\nslop_score (0-100) is how much of the trouble is shape rather than behavior:\n0 for a pure bug, 100 for pure slop."` | at most 4000 characters | organization | | What triage and the watcher are told slop, a bug and noise are, and what slop_score means, in their system prompts. Name each of kinds in it. | +| `review_guide` | text | `"## This change is a slop cleanup\n\nIt was judged slop, not a bug: it must keep behavior and improve shape.\nApprove only when all of these hold, and request changes otherwise, saying which:\n- behavior is unchanged: the tests pass, and no public behavior, API or output\n moved (removing what nothing reaches is fine);\n- the change removes or merges more than it adds, or is plainly simpler;\n- it adds no new guard, retry, fallback, catch-all or flag to hide a symptom."` | at most 2000 characters | organization | | What the reviewer of a change the Slopmaster made is told to check, before its usual instructions. The lines the branch adds and removes are said after it. | +| `ticket_heading` | text | `"Slop, not a bug"` | at most 80 characters | organization | | The heading, without its # marks, that the watcher's draft ticket for a finding judged slop opens with, in the tracker it files to. Triage is told to weigh a section under it. | +| `triage_lean_min_level` | whole number or null | `4` | ≥ 1, ≤ 5 | organization | | The Slop-O-Meter level (1 to 5) from which triage leans toward slop on a borderline verdict, told with the levels' names (slop_meter.level_names). Null: it is given the reading and never told to lean. | +| `dungeon_min_level` | whole number or null | `2` | ≥ 1, ≤ 5 | organization | | The Slop-O-Meter level (1 to 5) from which a repository or an area has a Slop Dungeon on the world map even with no open slop finding. Null: only open slop and recent cleanups make one. | + #### `ui` What the dashboard shows: the game layer, and the page it opens on. diff --git a/docs/examples/settings.yaml b/docs/examples/settings.yaml index c06295cc..d9ca3c6f 100644 --- a/docs/examples/settings.yaml +++ b/docs/examples/settings.yaml @@ -702,6 +702,92 @@ slop_meter: # inherits, as bots. At most 200 in all, counted with what a write inherits. # ai_agents: [] +# What embry0 does with a code job or a finding judged slop: whether and where +# it is routed, what the classifiers and the reviewer are told, its kinds, its +# ticket heading and the meter levels it acts on. +slop_policy: + # A code job judged slop runs route_template instead of the route triage + # picked. False: it takes its normal route, the verdict is still recorded on + # the job, and triage is told so. + # enabled: true + + # The pipeline template a code job judged slop runs. A write that changes + # it, by naming another or by taking the key out, must leave a template that + # takes a code job: one whose jobs are recorded as code jobs and whose route + # has a coding step. A template a document routes to cannot be deleted, + # renamed or changed to stop taking one. A job pinned to the full pipeline + # takes it only when its route also has review and QA. A template a person + # named for the job still wins. + # route_template: slop-cleanup + + # The least slop_score (0 to 100, how much of the trouble is shape) a slop + # verdict needs to be routed. 0 routes every slop verdict; one below it + # takes its normal route. + # min_score_to_route: 0 + + # The kinds of slop a verdict may name, as lowercase words joined by _. A + # kind a classifier names that is not here is dropped from its verdict. + # Describe each in classifier_guide, which is what the classifiers are told. + # kinds: + # - duplication + # - dead_code + # - half_wired + # - contradicting_layers + # - symptom_patch + # - misleading_docs + # - off_pattern + + # What triage and the watcher are told slop, a bug and noise are, and what + # slop_score means, in their system prompts. Name each of kinds in it. + # classifier_guide: |- + # Tell slop apart from bugs: + # - bug: the behavior differs from what the code, its docs or the ticket intend. + # Fixing it changes behavior. + # - slop: the behavior is what the code says, and the trouble is the code's + # shape. Cleaning it keeps behavior. Kinds: duplication (the same logic in + # more than one place), dead_code (paths, flags or files nothing reaches), + # half_wired (a feature or abstraction started and not finished), + # contradicting_layers (two layers that disagree about one thing), + # symptom_patch (guards, retries, fallbacks or catch-alls stacked on a symptom + # instead of its cause), misleading_docs (comments or docs that say what the + # code does not do), off_pattern (code that ignores the repository's own way). + # - noise: nothing to do. + # A guard, retry or catch-all stacked on a symptom is slop when removing it and + # fixing the shape keeps the outcome. When the code does the wrong thing (keeps + # retrying what can never succeed, drops or duplicates data, answers wrongly), + # it is a bug, even when sloppy code caused it: say bug, and name the slop in + # the evidence. Do not call a real defect slop to make it look smaller. + # slop_score (0-100) is how much of the trouble is shape rather than behavior: + # 0 for a pure bug, 100 for pure slop. + + # What the reviewer of a change the Slopmaster made is told to check, before + # its usual instructions. The lines the branch adds and removes are said + # after it. + # review_guide: |- + # ## This change is a slop cleanup + # + # It was judged slop, not a bug: it must keep behavior and improve shape. + # Approve only when all of these hold, and request changes otherwise, saying which: + # - behavior is unchanged: the tests pass, and no public behavior, API or output + # moved (removing what nothing reaches is fine); + # - the change removes or merges more than it adds, or is plainly simpler; + # - it adds no new guard, retry, fallback, catch-all or flag to hide a symptom. + + # The heading, without its # marks, that the watcher's draft ticket for a + # finding judged slop opens with, in the tracker it files to. Triage is told + # to weigh a section under it. + # ticket_heading: Slop, not a bug + + # The Slop-O-Meter level (1 to 5) from which triage leans toward slop on a + # borderline verdict, told with the levels' names (slop_meter.level_names). + # Null: it is given the reading and never told to lean. + # triage_lean_min_level: 4 + + # The Slop-O-Meter level (1 to 5) from which a repository or an area has a + # Slop Dungeon on the world map even with no open slop finding. Null: only + # open slop and recent cleanups make one. + # dungeon_min_level: 2 + # What the dashboard shows: the game layer, and the page it opens on. ui: # Show the game layer: the guild, the agents' levels, skills and outfits, diff --git a/docs/glossary.md b/docs/glossary.md index e279db3b..39fac27c 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -187,7 +187,7 @@ The job page's one-line status bar (`frontend/src/components/jobs/JobHud.tsx`, t The agent that closes the loop — merges the PR, performs [Distill](#distill) (extract reusable template) and [Coagulate](#coagulate) (ship + record). Substance sigils: Aqua Vitae 🜈 (Distill), Sol ☉ (Coagulate). ### Slopmaster -The `slopmaster` agent: the cleanup of slop, not the fix of a bug. A code job that triage judges [slop](#slop) runs the `slop-cleanup` template (triage → slopmaster → reviewer → qa → output), and its `slopmaster` step runs on the developer node, under its own agent type and definition: it works out what the code is meant to do, keeps that behavior (its tests prove it), deletes and merges instead of adding guards or retries, and opens a pull request like the developer. The reviewer approves a cleanup only when behavior is unchanged, the change shrinks or plainly simplifies the code, and it adds no guard, retry or fallback. A merged cleanup earns the Slopmaster its `cleaned` experience: 20,000 plus 300 per net line removed. Spec: `docs/superpowers/specs/2026-10-25-slop-dungeon-design.md`. (2026-10) +The `slopmaster` agent: the cleanup of slop, not the fix of a bug. A code job that triage judges [slop](#slop) runs the template its `slop_policy.route_template` names, by default `slop-cleanup` (triage → slopmaster → reviewer → qa → output; [slop routing](slop-routing.md)), and its `slopmaster` step runs on the developer node, under its own agent type and definition: it works out what the code is meant to do, keeps that behavior (its tests prove it), deletes and merges instead of adding guards or retries, and opens a pull request like the developer. The reviewer approves a cleanup only when behavior is unchanged, the change shrinks or plainly simplifies the code, and it adds no guard, retry or fallback. A merged cleanup earns the Slopmaster its `cleaned` experience: 20,000 plus 300 per net line removed. Spec: `docs/superpowers/specs/2026-10-25-slop-dungeon-design.md`. (2026-10) ### Slop What the slopometer calls a finding or a code job whose code does what it says, where the trouble is the code's shape: duplication, dead code, half-wired features, layers that contradict each other, symptom patches (guards, retries, catch-alls stacked on a symptom), docs that say what the code does not do, or code that ignores the repository's own patterns. The other natures are **bug** (behavior differs from what is intended) and **noise** (nothing to do). The watcher (its `NATURE:`/`SLOP_*` contract lines) and triage (its decision's `work_nature`) both classify, as `{nature, slop_score 0-100, kinds, evidence}`; the verdict is stored on the job and the case (`work_nature`, migration 118). A slop finding's draft ticket opens with a **Slop, not a bug** section, its proposal reads as a cleanup, and its job runs the [Slopmaster](#slopmaster). (2026-10) diff --git a/docs/slop-meter.md b/docs/slop-meter.md index ddf214e2..7e040d84 100644 --- a/docs/slop-meter.md +++ b/docs/slop-meter.md @@ -143,8 +143,8 @@ The loop is on by default and reads what is enrolled. `slop_meter.enabled: false ## Where it shows - `GET /api/v1/repos/{repo}/-/slop-meter` and the repository page: the score, the level and a week-by-week line of cost, capacity and slop ([api.md](api.md)). -- The Slop Dungeons in the [world](world.md) carry each area's reading; an area at level 2 or worse has a dungeon even with no open slop finding. -- Triage is told the job's area's (else the repository's) level, and the Slopmaster how much of the area landed beyond human attention. +- The Slop Dungeons in the [world](world.md) carry each area's reading; an area at `slop_policy.dungeon_min_level` or worse (level 2 by default) has a dungeon even with no open slop finding. +- Triage is told the job's area's (else the repository's) level and leans toward slop on a borderline verdict from `slop_policy.triage_lean_min_level` (Sloppy Joe by default), and the Slopmaster is told how much of the area landed beyond human attention. What a verdict of slop does to a job is [slop routing](slop-routing.md). ## Not covered diff --git a/docs/slop-routing.md b/docs/slop-routing.md new file mode 100644 index 00000000..a2ed3cad --- /dev/null +++ b/docs/slop-routing.md @@ -0,0 +1,102 @@ +# Slop routing + +Triage and the watcher judge each code job and each finding: a **bug** (the behavior differs from what is intended), **slop** (the behavior is what the code says, and the trouble is its shape) or **noise**. A developer handed slop as a bug patches the symptom and adds more slop, so a code job judged slop runs a cleanup instead: by default the `slop-cleanup` template, where the Slopmaster keeps behavior and removes or merges code, and the reviewer checks that it did ([the design](superpowers/specs/2026-10-25-slop-dungeon-design.md)). The [Slop-O-Meter](slop-meter.md)'s reading tips a borderline verdict toward slop, and gives an area a Slop Dungeon on the [world](world.md) map. + +What that verdict sets in motion is the settings document's `slop_policy` section ([settings document](configuration.md#settings-document); every key with its bounds is in the [reference](configuration.md#slop_policy)). Each default is what the code held before the section, so a deployment that writes nothing routes, prompts and files tickets exactly as before. + +## Settings + +| Key | Default | What it does | Layers | +|---|---|---|---| +| `enabled` | `true` | A code job judged slop runs `route_template`. `false`: it takes its normal route (triage's pick, else its kind's default); the verdict is still recorded on the job, and triage is told the verdict changes nothing | organization, repository | +| `route_template` | `slop-cleanup` | The pipeline template a code job judged slop runs. A write that changes it must leave a template that takes a code job ([below](#the-route-template)). A job pinned to the full pipeline keeps review and QA ([below](#a-job-pinned-to-the-full-pipeline)) | organization, repository | +| `min_score_to_route` | `0` | The least `slop_score` (0 to 100: how much of the trouble is shape) a slop verdict needs to be routed; below it the job takes its normal route. `0` routes every slop verdict | organization, repository | +| `kinds` | the seven below | The kinds of slop a verdict may name, as lowercase words joined by `_`. A kind a classifier names that is not listed is dropped from its verdict. At least one, at most 20 | organization | +| `classifier_guide` | the text below | What triage and the watcher are told slop, a bug and noise are, and what `slop_score` means. It is part of their system prompts; describe each of `kinds` in it. At most 4,000 characters | organization | +| `review_guide` | the text below | What the reviewer of the Slopmaster's change is told to check, before its usual instructions; the lines the branch adds and removes follow it. At most 2,000 characters | organization | +| `ticket_heading` | `Slop, not a bug` | The heading the watcher's draft ticket for a slop finding opens with, in the organization's tracker, written without its `#` marks. Triage is told to weigh a section under it | organization | +| `triage_lean_min_level` | `4` (Sloppy Joe) | The Slop-O-Meter level from which triage leans toward slop on a borderline verdict. Triage is told the levels by the names the meter gives them (`slop_meter.level_names`), so renamed levels read the same in both. `null`: triage is given the reading and never told to lean | organization | +| `dungeon_min_level` | `2` (Spaghetti Bolognese) | The Slop-O-Meter level from which a repository or an area has a Slop Dungeon on the world map even with no open slop finding. `null`: only open slop and cleanups merged lately make one | organization | + +The default kinds are `duplication`, `dead_code`, `half_wired`, `contradicting_layers`, `symptom_patch`, `misleading_docs` and `off_pattern`, each described in the default `classifier_guide`. The default texts are in the [example settings file](examples/settings.yaml) (`embry0 settings export --default` prints it). + +Examples: + +```yaml +# The deployment's document: route to an operator's own cleanup, and only sure verdicts. +slop_policy: + route_template: tidy-up + min_score_to_route: 60 +``` + +```yaml +# PUT /api/v1/config/settings?owner=acme: acme's own kinds, words and ticket heading. +slop_policy: + kinds: [duplication, dead_code, naming] + classifier_guide: |- + Tell slop apart from bugs: + - bug: the behavior differs from what the code, its docs or the ticket intend. + - slop: the behavior is intended, and the shape is the trouble. Kinds: + duplication (one logic in two places), dead_code (nothing reaches it), + naming (a name that says something the code does not do). + - noise: nothing to do. + slop_score (0-100) is how much of the trouble is shape rather than behavior. + ticket_heading: Shape, not behaviour + triage_lean_min_level: 3 +``` + +```yaml +# PATCH /api/v1/config/settings/slop_policy?repo=acme/legacy {"enabled": false}: this repository keeps its routes. +slop_policy: + enabled: false +``` + +Experience for a merged cleanup is the [progression pack](progression.md)'s `outcomes.cleaned` (a base, plus some per net line removed, up to a maximum), which an operator already sets there; it is not a key of this section. No environment variable ever set any of these keys, so nothing is imported at the first start and no variable is retired. + +## Layers + +Whether, where and how surely a job is routed are an organization's and a repository's to say, a repository's layer over its organization's, as for the background features ([which layer wins](configuration.md#settings-document)): a person can already name the pipeline of any job of a repository. The rest is an organization's own definition of slop, the bar its cleanups are reviewed against and how its tickets read. Those words go into agents' system prompts, so a repository's layer, which a key with a repository allowlist may write, holds none of them, and an organization's layer reaches its own repositories' jobs only. + +Two things follow from that order: + +- A repository's layer that sets `enabled: true` routes that repository's jobs judged slop even when its organization's layer sets `enabled: false`. +- A repository's layer that sets `route_template` holds that template against a delete or a rename ([below](#the-route-template)) until someone changes or takes out the key in that layer. + +## When it is read + +- **A job** reads the section for its repository once, when it starts, and keeps it to its end, a resume included, as it does `done_when` and `salvage`: triage's prompt, the route and the reviewer's guide come from what it read. A change reaches the next job that starts. +- **The watcher** reads its stack's repository's section for each analysis (the guide and the kinds) and for each draft it files (the heading), with no restart. +- **The world** reads each repository's `dungeon_min_level` each time it draws the dungeons. + +Triage's prompt is built from the section, with the default texts giving the prompt it held before to the byte. It is told that a slop verdict routes the job to the Slopmaster's cleanup when `route_template` is the built-in `slop-cleanup`, whose graph is written again at every start, and to the cleanup pipeline it names otherwise, whichever agent codes it. Each part is filled in one pass, so a guide that contains a marker such as `<>` is sent as written and never filled in. + +## The route template + +A write that changes the template a document's readers route to answers 422 naming `slop_policy.route_template` unless that template is one that takes a code job. That is a write naming another template, and also one that takes the key, or the whole section, out of the last document that set it, so its readers fall back to what they inherit or to the default `slop-cleanup`. Every write path is checked the same way: `PUT` and `PATCH /api/v1/config/settings` and their dry runs, `embry0 settings apply` and `set`, MCP `settings_set` and `settings_set_keys`, the Policy tab, and an undo. A template takes a code job when: + +- the template exists (`GET /api/v1/pipelines/templates` lists them; the pipelines the code defines, `qa`, `pr-review` and `onboard`, are not templates); +- its jobs are recorded as code jobs: it is the default of no other kind, says no other `job_kind`, and is not a route of specialists only, which is analysis (the template's row, [api.md](api.md#pipeline-templates)); +- its route has a coding step, `developer` or `slopmaster`. + +Only a template the write changes is checked, so a write of another key never trips over a template its readers already routed to, even one deleted since by hand (jobs then take their kind's default route, below). A layer that does not set `route_template` inherits the one above it. + +**While a document routes to a template, it stays one that can be routed to.** `DELETE /api/v1/pipelines/templates/{id}`, and a `PUT` that renames the template or changes it so that it no longer takes a code job, answer 409 naming each document that routes to it: the deployment's (`its default` when it sets no `route_template`, which names `slop-cleanup`), and each organization's or repository's layer that sets it. Point those documents at another template first; that write is recorded and can be undone like any other. Turning the policy off does not release the template: a document that names a template keeps it, so turning the policy on again does not route to a template deleted while it was off. + +- A rename does not rewrite the documents, as it moves an agent's `quest_template`: a document is the operator's text, possibly kept in a file that the next `embry0 settings apply` writes back. +- The check and the change cannot race: a settings write that may change `slop_policy` reads the templates `FOR SHARE` in its transaction (a write of any other section reads none and locks nothing), and a template's change locks its row `FOR UPDATE` before it reads the documents, so whichever comes second sees what the first committed. +- A template that is gone anyway, invalid, or no longer takes a code job (its row edited in the database by hand) is logged when a job would run it (`plan_route_template_not_found` with `slop_route`, `plan_route_template_invalid`, `plan_route_slop_template_unfit`), and the job takes the route its kind defaults to. A slop route never fails a job, unlike a template a person named that is invalid. + +### A job pinned to the full pipeline + +A job working a watcher-origin case is pinned to the full pipeline (`pin_full_pipeline`): it keeps review and QA. When it is judged slop, it takes `route_template` only when that template's route also has a review step (`reviewer` or `review`) and a QA step (`qa`), as `slop-cleanup` does; otherwise it is logged (`plan_route_slop_template_unfit`) and takes the code default, as for any template it cannot take. A job that is not pinned takes a lighter cleanup route, such as `quick-fix`, as its policy says. + +### The built-in templates + +The built-in templates, `slop-cleanup` among them, are written again at every start, as before: their graph and description are embry0's, while what their rows say about their jobs stays the operator's. Since routing is now `enabled` and `route_template`, deleting `slop-cleanup` is no longer how to stop routing, and a deployment that points `route_template` elsewhere leaves it unused. + +## What the routing logs + +- `plan_route_slop_cleanup`: a code job judged slop runs `template`, with its `slop_score`. +- `plan_route_slop_not_routed`: a code job judged slop keeps its route, with the `reason` (`slop_policy.enabled is false`, or a score below `min_score_to_route`). +- `pipeline_template_change_refused`: a template change refused, with the documents that route to it. +- `watcher_slop_policy_unread`: the watcher could not read its stack's policy and used the defaults for that analysis. diff --git a/embry0/api/app.py b/embry0/api/app.py index 2f88c469..add84119 100644 --- a/embry0/api/app.py +++ b/embry0/api/app.py @@ -669,6 +669,21 @@ async def read() -> list[dict[str, str]] | None: return read +def _watcher_slop_policy(app: FastAPI, repo: str) -> Any: + """``() -> policy``: the settings document's ``slop_policy`` as the stack's repository reads it now (#169). + + Read for each analysis and each filing, so a change applies to the next + one without a restart: what the analysis is told of slop, the kinds its + verdict may name and the heading of a slop finding's ticket. + """ + from embry0.settings.sections.slop_policy import SLOP_POLICY + + async def read() -> Any: + return (await app.state.settings.snapshot()).get(SLOP_POLICY, repo=repo or None) + + return read + + def _watcher_auto_acceptor(app: FastAPI) -> Any: """``(repo, bar) -> accepted``: accepts a stack's tracked drafts that clear its bar (spec 2026-10-25). @@ -2185,6 +2200,7 @@ def _watcher_for(stack: WatcherStack) -> WatcherService: auto_acceptor=auto_acceptor, local_first_model=_watcher_local_first_model(app, stack.repo), signature_masks=_watcher_signature_masks(app, stack.repo), + slop_policy=_watcher_slop_policy(app, stack.repo), cases_repo=app.state.healing_cases_repo, case_recorder=app.state.case_recorder, # #278: a deploy marker is reported on a case only after the forge diff --git a/embry0/api/v1/pipeline_templates.py b/embry0/api/v1/pipeline_templates.py index a1d24b76..d1bdfa81 100644 --- a/embry0/api/v1/pipeline_templates.py +++ b/embry0/api/v1/pipeline_templates.py @@ -12,8 +12,10 @@ from embry0.api.schemas import TemplateCreateRequest, TemplateDuplicateRequest, TemplateUpdateRequest from embry0.api.v1._skill_writes import check_added_skills from embry0.services.model_enablement import UnpricedModelError, ensure_models_priced +from embry0.services.slop_routing import TemplateInUse, slop_route_guard from embry0.services.world import forget_rows -from embry0.storage.repositories.pipeline_templates import PipelineTemplatesRepository +from embry0.storage.repositories.pipeline_templates import Guard, PipelineTemplatesRepository +from embry0.storage.repositories.settings_documents import SettingsDocumentsRepository from embry0.workflows._validation import validate_graph_definition from embry0.workflows.issue_to_pr.route_plan import node_config, step_run_type from embry0.workflows.row_behaviour import leader_problem @@ -166,6 +168,15 @@ def _fixed_route_warnings(fixed: bool, graph: dict[str, Any] | None) -> list[str ] +def _route_guard(request: Request) -> Guard | None: + """The guard a template's change runs: a template a settings document routes slop jobs to (#169) is not + deleted, renamed or made to stop taking a code job. None without the settings store (tests).""" + store = getattr(request.app.state, "settings", None) + if store is None: + return None + return slop_route_guard(SettingsDocumentsRepository(store.db), store.catalog) + + async def _check_template_skills(request: Request, graph: dict[str, Any] | None) -> list[str]: """Check every name the graph adds; return the warnings.""" return await check_added_skills(request, _added_skills(graph)) @@ -283,7 +294,9 @@ async def update_template( updates.get("graph_definition", stored.get("graph_definition")), ) try: - updated = await repo.update(template_id, **updates) + updated = await repo.update(template_id, guard=_route_guard(request), **updates) + except TemplateInUse as exc: + raise HTTPException(status_code=409, detail=str(exc)) from None except asyncpg.UniqueViolationError as exc: if exc.constraint_name == _DEFAULT_KIND_INDEX: raise _default_kind_taken(updates.get("default_for_kind")) from None @@ -303,7 +316,10 @@ async def delete_template( request: Request, repo: PipelineTemplatesRepository = Depends(get_templates_repo), ) -> dict[str, Any]: - await repo.delete(template_id) + try: + await repo.delete(template_id, guard=_route_guard(request)) + except TemplateInUse as exc: + raise HTTPException(status_code=409, detail=str(exc)) from None forget_rows(request.app.state, "templates") return {"id": template_id, "status": "deleted"} diff --git a/embry0/api/v1/world.py b/embry0/api/v1/world.py index 7361f885..32739b3b 100644 --- a/embry0/api/v1/world.py +++ b/embry0/api/v1/world.py @@ -111,6 +111,23 @@ async def _leads(request: Request) -> list[dict[str, Any]]: return [] +async def _dungeon_levels(request: Request) -> Callable[[str], int | None]: + """``repo -> level``: the Slop-O-Meter level from which each repository's areas have a dungeon on their meter + alone, its ``slop_policy.dungeon_min_level`` (#169); the section's default when the settings cannot be read.""" + from embry0.settings.sections.slop_policy import SLOP_POLICY + + default = SLOP_POLICY.defaults().dungeon_min_level + store = getattr(request.app.state, "settings", None) + if store is None: + return lambda _repo: default + try: + scoped = (await store.snapshot()).scoped(SLOP_POLICY) + except Exception: # noqa: BLE001 - a dungeon is still drawn at the default level + logger.warning("world_slop_policy_unread", exc_info=True) + return lambda _repo: default + return lambda repo: scoped.repo(repo).dungeon_min_level + + async def _meters(request: Request) -> list[dict[str, Any]]: """Every Slop-O-Meter score, by repository and area (none without the store).""" store = getattr(request.app.state, "slop_meter_repo", None) @@ -266,7 +283,7 @@ async def read_world(request: Request) -> dict[str, Any]: "quests": quests, # Slop dungeon (spec 2026-10-25): each repository's and area's open # slop, its score, and the cleanups merged lately. - "dungeons": dungeons(quests, cleaned, meter_rows), + "dungeons": dungeons(quests, cleaned, meter_rows, min_level=await _dungeon_levels(request)), "inbox": [ { "job_id": r["job_id"], diff --git a/embry0/orchestration/nodes/triage.py b/embry0/orchestration/nodes/triage.py index f210ca58..4991ed4f 100644 --- a/embry0/orchestration/nodes/triage.py +++ b/embry0/orchestration/nodes/triage.py @@ -8,6 +8,7 @@ import json import re +import textwrap from collections.abc import Mapping, Sequence from typing import Any, cast @@ -16,7 +17,8 @@ from embry0.agents.model_policy import TIER_KEYS from embry0.orchestration.state import TriageDecision from embry0.services.done_when import done_when_guide -from embry0.services.slop import CLASSIFIER_GUIDE +from embry0.services.slop import lean_names, level_phrase +from embry0.settings.sections.slop_policy import DEFAULT_ROUTE_TEMPLATE, SlopPolicySettings logger = structlog.get_logger(__name__) @@ -29,6 +31,12 @@ # Where the developer's skill selection goes: the skills its row lets triage # add (agent_definitions.triage_may_add_skills), or nothing when it lets none. _SKILL_SELECTION_SLOT = "<>" +# Where the "Bug or slop" section's guide and its routing paragraph go: the +# job's slop_policy (#169), what its organization tells the classifiers. +_SLOP_GUIDE_SLOT = "<>" +_SLOP_ROUTING_SLOT = "<>" +#: A slot of the template: every one is filled in one pass (:func:`_fill_slots`). +_SLOT = re.compile(r"<<[A-Z_]+>>") # What the prompt says of a skill triage may add. A skill not named here gets # a line of its own (_GENERIC_SKILL_GUIDANCE). @@ -98,14 +106,7 @@ nature. A developer handed slop as a bug patches the symptom and adds more slop, so this verdict decides who works the job and how they are briefed. <> -Fill `work_nature` for every code job, with at most 8 evidence entries naming -files you read. When the task carries a watcher "Slop, not a bug" section, -weigh it, then make your own call. A slop verdict routes the job to the -Slopmaster's cleanup (keep behavior, remove or merge code) whatever template -you name; leave pipeline_template empty for it. The task context may give the -area's Slop-O-Meter reading (how much of its code landed beyond human -attention); when your verdict is borderline and it reads Sloppy Joe or Just -The Slop, lean toward slop. +<> <> @@ -239,7 +240,7 @@ Omit `qa_failure_action` entirely on the initial invocation (when QA hasn't run yet). -Respond ONLY with the JSON object, no markdown fences or extra text.""".replace("<>", CLASSIFIER_GUIDE) +Respond ONLY with the JSON object, no markdown fences or extra text.""" _SUB_AGENT_MODELS_NOTE = """\ - The developer agent itself can dispatch sub-agents (via the Agent tool) for @@ -323,26 +324,88 @@ def build_skill_selection(skill_choices: Sequence[str]) -> str: return section +def build_slop_routing(slop_policy: SlopPolicySettings, level_names: Sequence[str]) -> str: + """The paragraph after the guide in "Bug or slop": what a verdict does to the job's route, and when the + Slop-O-Meter's reading tips a borderline one, as the job's ``slop_policy`` has it. + + ``level_names`` are the meter's (``slop_meter.level_names`` of the job's + repository). Wrapped at 78 columns, as the prompt is written, so the + defaults give the paragraph the prompt held before the policy existed, + to the byte. + """ + sentences = [ + "Fill `work_nature` for every code job, with at most 8 evidence entries naming files you read.", + f'When the task carries a watcher "{slop_policy.ticket_heading}" section, weigh it, then make your own call.', + ] + if not slop_policy.enabled: + sentences.append("A slop verdict does not change the job's route here: pick its template as for a bug.") + else: + floor = slop_policy.min_score_to_route + verdict = f"A slop verdict with a slop_score of {floor} or more" if floor else "A slop verdict" + sentences.append( + f"{verdict} routes the job to {_cleanup_route(slop_policy.route_template)} (keep behavior, remove or " + "merge code) whatever template you name; leave pipeline_template empty for it." + ) + if floor: + sentences.append("Below that score, pick its template as for a bug.") + reading = ( + "The task context may give the area's Slop-O-Meter reading (how much of its code landed beyond human attention)" + ) + leaning = lean_names(level_names, slop_policy.triage_lean_min_level) + if leaning: + sentences.append( + f"{reading}; when your verdict is borderline and it reads {level_phrase(leaning)}, lean toward slop." + ) + else: + sentences.append(f"{reading}.") + return textwrap.fill(" ".join(sentences), width=78, break_long_words=False, break_on_hyphens=False) + + +def _cleanup_route(template: str) -> str: + """What the slop route is called in the prompt: the built-in cleanup is the Slopmaster's (its graph is the + seed's, written again at every start); any other is named, whoever codes it.""" + if template == DEFAULT_ROUTE_TEMPLATE: + return "the Slopmaster's cleanup" + return f"the cleanup pipeline `{template}`" + + +def _fill_slots(template: str, slots: Mapping[str, str]) -> str: + """``template`` with each of ``slots`` in its place, in one pass: what one slot holds (an operator's guide, + a heading, a skill's name) is never read for another slot's marker.""" + return _SLOT.sub(lambda found: slots.get(found.group(0), found.group(0)), template) + + def build_triage_system_prompt( *, default_model: str, tier_models: Mapping[str, str], ticket_sections: Sequence[str], + slop_policy: SlopPolicySettings, + level_names: Sequence[str], skill_choices: Sequence[str] = (), ) -> str: - """The triage system prompt for one deployment configuration and one repository's ticket headings. - - ``ticket_sections`` is the job's ``done_when.ticket_sections``. - ``skill_choices`` are the skills triage may add to the developer - (``build_skill_selection``); none leaves the skill selection out. - Identical for every job of a repository while the provider settings, - that section and the developer's row are unchanged, so the CLI's prompt - caching still covers it (EMB-35). + """The triage system prompt for one deployment configuration and one repository's settings. + + ``ticket_sections`` is the job's ``done_when.ticket_sections``, + ``slop_policy`` its ``slop_policy`` section (what the classifier is told + and what a verdict does to the route) and ``level_names`` the + Slop-O-Meter's level names for its repository. ``skill_choices`` are the + skills triage may add to the developer (``build_skill_selection``); none + leaves the skill selection out. Identical for every job of a repository + while the provider settings, those sections and the developer's row are + unchanged, so the CLI's prompt caching still covers it (EMB-35). """ guidance = build_triage_model_guidance(default_model=default_model, tier_models=tier_models) - prompt = _TRIAGE_SYSTEM_PROMPT_TEMPLATE.replace(_MODEL_GUIDANCE_SLOT, guidance) - prompt = prompt.replace(_SKILL_SELECTION_SLOT, build_skill_selection(skill_choices)) - return prompt.replace(_DONE_WHEN_GUIDE_SLOT, done_when_guide(ticket_sections)) + return _fill_slots( + _TRIAGE_SYSTEM_PROMPT_TEMPLATE, + { + _SLOP_GUIDE_SLOT: slop_policy.classifier_guide, + _SLOP_ROUTING_SLOT: build_slop_routing(slop_policy, level_names), + _DONE_WHEN_GUIDE_SLOT: done_when_guide(ticket_sections), + _MODEL_GUIDANCE_SLOT: guidance, + _SKILL_SELECTION_SLOT: build_skill_selection(skill_choices), + }, + ) def _extract_json_payload(raw: str) -> str: diff --git a/embry0/services/job_settings.py b/embry0/services/job_settings.py index 3a56fb10..d6b2e730 100644 --- a/embry0/services/job_settings.py +++ b/embry0/services/job_settings.py @@ -4,7 +4,9 @@ repository: the deployment's values, then its organization's layer, then its own. So is the ``context_audit`` section's choice of instruction files, which the context auditor's command is written from when its task carries none -(#170). A job reads them for its repository when it starts (``_run_workflow``, +(#170), and the ``slop_policy`` section, which says what triage is told of +slop, where a code job judged slop is routed and what its reviewer checks +(#169). A job reads them for its repository when it starts (``_run_workflow``, one snapshot) and keeps what it read on its state, under :data:`STATE_KEY`. The state is checkpointed, so every node of the job reads the same values, and so does the job after a pause, an answer or a restart: a change to the @@ -30,6 +32,7 @@ from embry0.settings.sections.context_audit import CONTEXT_AUDIT from embry0.settings.sections.done_when import DONE_WHEN from embry0.settings.sections.salvage import SALVAGE +from embry0.settings.sections.slop_policy import SLOP_POLICY if TYPE_CHECKING: from embry0.settings.store import SettingsStore @@ -37,7 +40,7 @@ logger = structlog.get_logger(__name__) #: The sections a job reads when it starts. -JOB_SECTIONS: Final[tuple[Section[Any], ...]] = (CONTEXT_AUDIT, DONE_WHEN, SALVAGE) +JOB_SECTIONS: Final[tuple[Section[Any], ...]] = (CONTEXT_AUDIT, DONE_WHEN, SALVAGE, SLOP_POLICY) #: Where a job keeps them: a ``JobState`` key, each section's values as plain JSON under its name. STATE_KEY: Final = "job_settings" diff --git a/embry0/services/proposals.py b/embry0/services/proposals.py index b82af307..be1cf277 100644 --- a/embry0/services/proposals.py +++ b/embry0/services/proposals.py @@ -201,7 +201,9 @@ def case_proposal(row: Mapping[str, Any]) -> Proposal: extra: dict[str, Any] = {"severity": severity, "occurrences": seen, "signature": row.get("signature")} # Slop dungeon (spec 2026-10-25): a finding the watcher judged slop reads # as a cleanup, so the owner is not asked to "fix" what is shape, not a bug. - verdict = parse_work_nature(row.get("work_nature")) + # The verdict was parsed with its stack's slop_policy.kinds when it was + # stored: every kind it kept is read back (#169). + verdict = parse_work_nature(row.get("work_nature"), kinds=None) if verdict is not None: extra.update(nature=verdict["nature"], slop_score=verdict["slop_score"], slop_kinds=verdict["kinds"]) if is_slop(verdict): diff --git a/embry0/services/slop.py b/embry0/services/slop.py index 29aea2e3..f4c12c23 100644 --- a/embry0/services/slop.py +++ b/embry0/services/slop.py @@ -8,58 +8,32 @@ runs as a cleanup by the Slopmaster. This module holds the one shape of that verdict (the slopometer), its lenient -parser, the words the classifiers and the Slopmaster are given, and the Slop -Dungeon's score. +parser, the words the Slopmaster and a slop finding's ticket are given, and +the Slop Dungeon's score. What the policy decides (the kinds a verdict may +name, what the classifiers and the reviewer are told, the ticket's heading, +the route and the meter levels acted on) is the settings document's +``slop_policy`` section (:mod:`embry0.settings.sections.slop_policy`): each +function here takes the part it needs from the caller, who read the section +for the job's or the finding's repository. """ from __future__ import annotations import math import re -from collections.abc import Iterable, Mapping +from collections.abc import Callable, Collection, Iterable, Mapping from datetime import date from typing import Any +from embry0.settings.sections.slop_policy import KIND, MAX_KIND_LENGTH + NATURES: tuple[str, ...] = ("bug", "slop", "noise") -SLOP_KINDS: tuple[str, ...] = ( - "duplication", - "dead_code", - "half_wired", - "contradicting_layers", - "symptom_patch", - "misleading_docs", - "off_pattern", -) MAX_EVIDENCE = 8 _WHY_CHARS = 300 _PATH_CHARS = 300 #: The agent that cleans slop, on the developer node (row_behaviour.CODING_AGENT_TYPES). SLOPMASTER = "slopmaster" -#: The template a slop job runs (storage/seeds/pipeline_templates_builtin.py). -SLOP_TEMPLATE = "slop-cleanup" - -#: What the classifiers are told slop is: triage's prompt and the watcher's contract both quote it. -CLASSIFIER_GUIDE = """\ -Tell slop apart from bugs: -- bug: the behavior differs from what the code, its docs or the ticket intend. - Fixing it changes behavior. -- slop: the behavior is what the code says, and the trouble is the code's - shape. Cleaning it keeps behavior. Kinds: duplication (the same logic in - more than one place), dead_code (paths, flags or files nothing reaches), - half_wired (a feature or abstraction started and not finished), - contradicting_layers (two layers that disagree about one thing), - symptom_patch (guards, retries, fallbacks or catch-alls stacked on a symptom - instead of its cause), misleading_docs (comments or docs that say what the - code does not do), off_pattern (code that ignores the repository's own way). -- noise: nothing to do. -A guard, retry or catch-all stacked on a symptom is slop when removing it and -fixing the shape keeps the outcome. When the code does the wrong thing (keeps -retrying what can never succeed, drops or duplicates data, answers wrongly), -it is a bug, even when sloppy code caused it: say bug, and name the slop in -the evidence. Do not call a real defect slop to make it look smaller. -slop_score (0-100) is how much of the trouble is shape rather than behavior: -0 for a pure bug, 100 for pure slop.""" #: The Slopmaster's system prompt (its builtin agent definition). SLOPMASTER_SYSTEM_PROMPT = """\ @@ -84,17 +58,6 @@ Your summary says what was slop, what you removed or merged, and how the tests show that behavior did not change.""" -#: What the reviewer is told when the change under review is a slop cleanup. -SLOP_REVIEW_GUIDE = """\ -## This change is a slop cleanup - -It was judged slop, not a bug: it must keep behavior and improve shape. -Approve only when all of these hold, and request changes otherwise, saying which: -- behavior is unchanged: the tests pass, and no public behavior, API or output - moved (removing what nothing reaches is fine); -- the change removes or merges more than it adds, or is plainly simpler; -- it adds no new guard, retry, fallback, catch-all or flag to hide a symptom.""" - def _clamp_score(value: Any) -> int | None: if isinstance(value, bool): @@ -108,13 +71,22 @@ def _clamp_score(value: Any) -> int | None: return max(0, min(100, int(round(number)))) -def parse_work_nature(raw: Any) -> dict[str, Any] | None: +def _kind(raw: Any) -> str: + """A kind as a verdict names it once read: lowercase, ``-`` and spaces as ``_``.""" + return str(raw).strip().lower().replace("-", "_").replace(" ", "_") + + +def parse_work_nature(raw: Any, *, kinds: Collection[str] | None) -> dict[str, Any] | None: """The slopometer's verdict in its one shape, or None when there is none. Lenient on purpose: a classifier's verdict steers routing and framing, it - never fails a job. A nature outside bug/slop/noise means no verdict; an - unknown kind is dropped; a score is clamped to 0..100 (a slop verdict with - no score reads as 100, a bug as 0); evidence keeps its first entries. + never fails a job. A nature outside bug/slop/noise means no verdict; a + kind not in ``kinds`` (the ``slop_policy.kinds`` of the job's or the + finding's repository) is dropped; a score is clamped to 0..100 (a slop + verdict with no score reads as 100, a bug as 0); evidence keeps its first + entries. ``kinds=None`` reads back a verdict stored after it was parsed: + each kind it names is kept when it has a kind's shape, so a kind an + organization added is not dropped by a reader of another. """ if not isinstance(raw, Mapping): return None @@ -127,11 +99,12 @@ def parse_work_nature(raw: Any) -> dict[str, Any] | None: kinds_raw = raw.get("kinds") or [] if isinstance(kinds_raw, str): kinds_raw = [k for k in re.split(r"[,\s]+", kinds_raw) if k] - kinds: list[str] = [] + named: list[str] = [] for kind in kinds_raw if isinstance(kinds_raw, list) else []: - k = str(kind).strip().lower().replace("-", "_").replace(" ", "_") - if k in SLOP_KINDS and k not in kinds: - kinds.append(k) + k = _kind(kind) + known = k in kinds if kinds is not None else len(k) <= MAX_KIND_LENGTH and KIND.fullmatch(k) is not None + if known and k not in named: + named.append(k) evidence: list[dict[str, str]] = [] items = raw.get("evidence") for item in items if isinstance(items, list) else []: @@ -143,7 +116,7 @@ def parse_work_nature(raw: Any) -> dict[str, Any] | None: evidence.append({"path": path, "why": why}) if len(evidence) >= MAX_EVIDENCE: break - verdict: dict[str, Any] = {"nature": nature, "slop_score": score, "kinds": kinds, "evidence": evidence} + verdict: dict[str, Any] = {"nature": nature, "slop_score": score, "kinds": named, "evidence": evidence} cleanup = raw.get("cleanup") if isinstance(cleanup, Mapping): counts = {k: int(cleanup[k]) for k in ("added", "removed") if isinstance(cleanup.get(k), int)} @@ -164,8 +137,8 @@ def is_slop(verdict: Mapping[str, Any] | None) -> bool: } -def parse_contract_lines(text: str) -> dict[str, Any] | None: - """The watcher's NATURE:/SLOP_SCORE:/SLOP_KINDS:/SLOP_EVIDENCE: lines, as a verdict. +def parse_contract_lines(text: str, *, kinds: Collection[str]) -> dict[str, Any] | None: + """The watcher's NATURE:/SLOP_SCORE:/SLOP_KINDS:/SLOP_EVIDENCE: lines, as a verdict naming only ``kinds``. Read from the head of a proposal only (before ``BODY:``), so a body that quotes these words is not read as a verdict. None without a NATURE line. @@ -175,15 +148,16 @@ def parse_contract_lines(text: str) -> dict[str, Any] | None: if nature is None: return None score = _LINE_RE["score"].search(head) - kinds = _LINE_RE["kinds"].search(head) + named = _LINE_RE["kinds"].search(head) evidence = [{"path": m.group(1).strip(), "why": m.group(2).strip()} for m in _LINE_RE["evidence"].finditer(head)] return parse_work_nature( { "nature": nature.group(1), "slop_score": int(score.group(1)) if score else None, - "kinds": kinds.group(1) if kinds else [], + "kinds": named.group(1) if named else [], "evidence": evidence, - } + }, + kinds=kinds, ) @@ -199,10 +173,10 @@ def _kinds_words(verdict: Mapping[str, Any]) -> str: return ", ".join(str(k).replace("_", " ") for k in verdict.get("kinds") or []) or "unspecified" -def ticket_section(verdict: Mapping[str, Any]) -> str: - """The **Slop, not a bug** section a slop finding's draft ticket opens with.""" +def ticket_section(verdict: Mapping[str, Any], *, heading: str) -> str: + """The section a slop finding's draft ticket opens with, under ``heading`` (``slop_policy.ticket_heading``).""" lines = [ - "## Slop, not a bug", + f"## {heading}", f"The watcher judged this slop (slop score {verdict.get('slop_score')}/100: {_kinds_words(verdict)}). " "The code does what it says; the trouble is its shape. Clean it up — keep behavior, remove or merge " "code — instead of adding another guard or retry.", @@ -240,8 +214,19 @@ def dungeon_score(scores: Iterable[int]) -> int: return int(round(100 * (1 - clean))) -#: The Slop-O-Meter level at which an area has a dungeon even with no open slop finding (Spaghetti Bolognese). -METER_DUNGEON_LEVEL = 2 +def lean_names(level_names: Iterable[str], min_level: int | None) -> tuple[str, ...]: + """The Slop-O-Meter level names from ``min_level`` up (``slop_policy.triage_lean_min_level``); none for None.""" + if min_level is None: + return () + return tuple(level_names)[min_level - 1 :] + + +def level_phrase(names: Iterable[str]) -> str: + """Level names as a sentence names them: ``A``, ``A or B``, ``A, B or C``.""" + words = list(names) + if len(words) <= 1: + return "".join(words) + return f"{', '.join(words[:-1])} or {words[-1]}" def meter_view(row: Mapping[str, Any] | None) -> dict[str, Any] | None: @@ -261,13 +246,17 @@ def meter_view(row: Mapping[str, Any] | None) -> dict[str, Any] | None: def dungeons( leads: Iterable[Mapping[str, Any]], cleaned: Mapping[tuple[str, str | None], int], - meters: Iterable[Mapping[str, Any]] = (), + meters: Iterable[Mapping[str, Any]], + *, + min_level: Callable[[str], int | None], ) -> list[dict[str, Any]]: """One Slop Dungeon per repository and area that has open slop, recent cleanups or a sloppy meter, worst first. - Each carries its Slop-O-Meter reading (``meter``, None when not scored); - an area at Spaghetti Bolognese or worse has a dungeon even with no open - slop finding. + Each carries its Slop-O-Meter reading (``meter``, None when not scored). + An area whose reading is at its repository's ``min_level`` or worse + (``slop_policy.dungeon_min_level``, Spaghetti Bolognese by default) has + a dungeon even with no open slop finding; with None, its meter alone + makes none. """ open_scores: dict[tuple[str, str | None], list[int]] = {} for lead in leads: @@ -277,7 +266,7 @@ def dungeons( open_scores.setdefault(key, []).append(int(lead.get("slop_score") or 0)) by_scope = {(str(m["repo"]), m.get("area") or None): m for m in meters} keys = set(open_scores) | {k for k, n in cleaned.items() if n} - keys |= {k for k, m in by_scope.items() if int(m.get("level") or 1) >= METER_DUNGEON_LEVEL} + keys |= {k for k, m in by_scope.items() if _sloppy(int(m.get("level") or 1), min_level(k[0]))} out = [ { "repo": repo, @@ -290,3 +279,7 @@ def dungeons( for repo, area in keys ] return sorted(out, key=lambda d: (-d["score"], -d["open"], d["repo"], d["area"] or "")) + + +def _sloppy(level: int, min_level: int | None) -> bool: + return min_level is not None and level >= min_level diff --git a/embry0/services/slop_meter/context.py b/embry0/services/slop_meter/context.py index 16adea01..05f6728c 100644 --- a/embry0/services/slop_meter/context.py +++ b/embry0/services/slop_meter/context.py @@ -24,6 +24,28 @@ def meter_sentence(scope: str, row: Mapping[str, Any]) -> str: ) +async def level_names(config: Any, repo: str) -> tuple[str, ...]: + """The meter's level names as ``repo`` reads them now (``slop_meter.level_names``), which triage's lean names. + + Read when triage writes its prompt, as the reading itself is, so the + names it leans by are the names its reading is given in. The defaults + without a database, and when the settings cannot be read. + """ + from embry0.settings.sections.slop_meter import SLOP_METER + + configurable = (config or {}).get("configurable", {}) if isinstance(config, Mapping) else {} + db = configurable.get("db") + if db is None: + return SLOP_METER.defaults().level_names + try: + from embry0.settings.store import SettingsStore + + return (await SettingsStore(db).snapshot()).get(SLOP_METER, repo=repo or None).level_names + except Exception: # noqa: BLE001 - the names are words of a prompt, never a failure + logger.info("slop_meter_level_names_unread", repo=repo, exc_info=True) + return SLOP_METER.defaults().level_names + + async def job_meter(config: Any, state: Mapping[str, Any]) -> tuple[str, dict[str, Any]] | None: """``(scope, score row)`` for the job: its area's when scored, else its repository's; None without one.""" configurable = (config or {}).get("configurable", {}) if isinstance(config, Mapping) else {} diff --git a/embry0/services/slop_routing.py b/embry0/services/slop_routing.py new file mode 100644 index 00000000..085f5d86 --- /dev/null +++ b/embry0/services/slop_routing.py @@ -0,0 +1,116 @@ +"""A pipeline template a slop policy routes to stays one it can route to (#169). + +A settings document's ``slop_policy.route_template`` names the template a +code job judged slop runs. A write of the key is checked against the +templates (``embry0.settings.write_context``: it must name one that takes a +code job). This is the other side: while any document routes to a template, +a change that would leave it naming one that is gone, or one that no longer +takes a code job, is refused (``DELETE`` and ``PUT /pipelines/templates`` +answer 409 with the documents that name it), so an operator points those +documents elsewhere first, and that write is recorded and undoable like any +other. + +**Refused, not carried along.** A rename does not rewrite the documents +(as it carries an agent's ``quest_template``, a column): a document is the +operator's text, possibly kept in a file that ``embry0 settings apply`` +writes back, and an automatic write to it would be undone, or refused, by +the next apply. + +**Counted with what each document reads.** A document routes to a template +when the policy it resolves to names it: the deployment's (its default +``slop-cleanup`` included, while it sets no other), and each layer that sets +``route_template`` itself. ``enabled`` is not consulted: turning a policy +back on is a write that does not name the template again, so it is never +checked again; a document that names a template keeps it. + +**No race.** The change runs in the template's transaction after its row is +locked ``FOR UPDATE``, and reads the documents there; a settings write +reads the templates ``FOR SHARE`` in its own transaction. Whichever comes +second waits for the first and sees what it committed. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any + +import asyncpg +import structlog + +from embry0.settings.registry import Catalog +from embry0.settings.scope import DEPLOYMENT, Scope, ScopeError +from embry0.settings.sections.slop_policy import SLOP_POLICY +from embry0.storage.repositories.pipeline_templates import Guard +from embry0.storage.repositories.settings_documents import SettingsDocumentsRepository +from embry0.workflows.row_behaviour import code_job_problem + +logger = structlog.get_logger(__name__) + + +class TemplateInUse(Exception): + """A template change refused: a settings document routes slop jobs to the template. The message names + the documents and what to do; it is the API's 409.""" + + +async def documents_routing_to( + conn: asyncpg.Connection, documents: SettingsDocumentsRepository, catalog: Catalog, name: str +) -> list[str]: + """The settings documents whose ``slop_policy`` routes a slop job to the template ``name``, read on ``conn``. + + Each as a person names it: ``the deployment's document`` (``its default`` + when it sets no ``route_template``), ``organization acme``, ``repository + acme/widgets``. Empty when the catalog has no ``slop_policy``. + """ + if SLOP_POLICY.name not in catalog: + return [] + from embry0.settings.store import build_snapshot + + snapshot = build_snapshot(catalog, await documents.all(conn), token=(), log=False) + holders: list[str] = [] + if snapshot.get(SLOP_POLICY).route_template == name: + own = "route_template" in (snapshot.data.get(SLOP_POLICY.name) or {}) + holders.append("the deployment's document" + ("" if own else " (its default)")) + for stored, data in sorted(snapshot.layers.items()): + if "route_template" not in (data.get(SLOP_POLICY.name) or {}): + continue + try: + scope = Scope.parse(stored) + except ScopeError: + continue + if scope.layer == DEPLOYMENT: + continue + model = ( + snapshot.get(SLOP_POLICY, owner=scope.name) + if scope.layer == "organization" + else snapshot.get(SLOP_POLICY, repo=scope.name) + ) + if model.route_template == name: + holders.append(f"{scope.layer} {scope.name}") + return holders + + +def slop_route_guard(documents: SettingsDocumentsRepository, catalog: Catalog) -> Guard: + """The guard a template's change runs: refuse a delete, a rename, or a change that makes the template stop + taking a code job (``row_behaviour.code_job_problem``), while a document routes slop jobs to it.""" + + async def guard(conn: asyncpg.Connection, before: Mapping[str, Any], after: Mapping[str, Any] | None) -> None: + name = str(before.get("name") or "") + if after is None: + change = "delete it" + elif str(after.get("name") or "") != name: + change = f"rename it to {str(after.get('name') or '')!r}" + elif code_job_problem(before) is None and (problem := code_job_problem(after)) is not None: + change = f"change it so that it no longer takes a code job ({problem})" + else: + return + holders = await documents_routing_to(conn, documents, catalog, name) + if not holders: + return + logger.info("pipeline_template_change_refused", template=name, change=change, held_by=holders) + raise TemplateInUse( + f"slop_policy.route_template sends code jobs judged slop to {name!r} in {', '.join(holders)}, so " + f"you cannot {change}. Set slop_policy.route_template there to another template first " + "(PATCH /api/v1/config/settings/slop_policy, with ?owner= or ?repo= for a layer)." + ) + + return guard diff --git a/embry0/services/watcher.py b/embry0/services/watcher.py index a4ab4857..8bdc0527 100644 --- a/embry0/services/watcher.py +++ b/embry0/services/watcher.py @@ -43,7 +43,7 @@ import json import re import uuid -from collections.abc import Awaitable, Callable, Sequence +from collections.abc import Awaitable, Callable, Collection, Sequence from datetime import UTC, datetime, timedelta from typing import Any, cast @@ -82,7 +82,6 @@ classify_window, ) from embry0.services.slop import ( - CLASSIFIER_GUIDE, WATCHER_CONTRACT_LINES, is_slop, parse_contract_lines, @@ -90,6 +89,7 @@ ) from embry0.services.watcher_budget import resolve_watcher_job_budget from embry0.services.watcher_recursion import drop_own_lines +from embry0.settings.sections.slop_policy import DEFAULT_KINDS, SLOP_POLICY, SlopPolicySettings from embry0.storage.repositories.watcher_runs import ANALYSED_ACTIONS, UNPARSEABLE_DETAIL_PREFIX from embry0.trackers.filing import DRAFT_TITLE_PREFIX @@ -249,8 +249,8 @@ def proposal_fingerprint(title: str) -> str: ) -def _parse_line_format(text: str) -> dict[str, Any] | None: - """The RAV-1062 TITLE:/SEVERITY:/BODY: plain-text proposal, or None.""" +def _parse_line_format(text: str, *, kinds: Collection[str] = DEFAULT_KINDS) -> dict[str, Any] | None: + """The RAV-1062 TITLE:/SEVERITY:/BODY: plain-text proposal, or None; its verdict names only ``kinds``.""" title_m = _TITLE_RE.search(text) body_m = _BODY_RE.search(text) if not (title_m and body_m): @@ -274,7 +274,7 @@ def _parse_line_format(text: str) -> dict[str, Any] | None: "severity": severity if severity in _SEVERITIES else "medium", "fingerprint_hint": (fp_m.group(1).strip()[:200] if fp_m else None), # Slop dungeon: the NATURE:/SLOP_* lines before BODY:, None without them. - "work_nature": parse_contract_lines(text), + "work_nature": parse_contract_lines(text, kinds=kinds), } @@ -299,9 +299,12 @@ def _parse_json_fallback(text: str) -> dict[str, Any] | None: return None -def parse_proposal(text: str) -> dict[str, Any] | None: +def parse_proposal(text: str, *, kinds: Collection[str] = DEFAULT_KINDS) -> dict[str, Any] | None: """The agent's deliverable → proposal dict, or None when it carries none. + ``kinds`` are the kinds of slop its verdict may name: the stack's + ``slop_policy.kinds`` (#169). + Primary contract (RAV-1062) is the TITLE:/SEVERITY:/BODY: line format — log-analysis bodies quote log lines that routinely contain JSON, and a JSON deliverable made the agent responsible for escaping them (it fumbled @@ -314,7 +317,7 @@ def parse_proposal(text: str) -> dict[str, Any] | None: """ if not text or text.strip().upper().startswith("NO_ISSUE"): return None - return _parse_line_format(text) or _parse_json_fallback(text) + return _parse_line_format(text, kinds=kinds) or _parse_json_fallback(text) def _unparseable_reason(text: str) -> str: @@ -349,9 +352,12 @@ def _unparseable_reason(text: str) -> str: """ -def parse_verdict(text: str) -> dict[str, Any]: +def parse_verdict(text: str, *, kinds: Collection[str] = DEFAULT_KINDS) -> dict[str, Any]: """The agent's deliverable → three-way verdict with rationale + memory. + A proposal's slop verdict names only ``kinds``, the stack's + ``slop_policy.kinds`` (#169). + ``kind`` is ``"no_issue"``, ``"proposal"`` or ``"unparseable"`` (SH-P2.4): output matching neither contract shape used to read as NO_ISSUE, a silent miss — it is now its own outcome, with ``reason`` saying what was wrong. @@ -367,7 +373,7 @@ def parse_verdict(text: str) -> dict[str, Any]: reason: str | None = None if text.strip().upper().startswith("NO_ISSUE"): kind = "no_issue" - elif (proposal := _parse_line_format(text)) is not None: + elif (proposal := _parse_line_format(text, kinds=kinds)) is not None: kind = "proposal" elif _NO_ISSUE_VERDICT_RE.search(text): kind = "no_issue" @@ -534,9 +540,15 @@ def __init__( # PUT applies to the next one without a restart. None: the masks the # stack had when this watcher was built (``config``), for good. signature_masks: Callable[[], Awaitable[Any]] | None = None, + # #169: ``() -> policy``, the settings document's slop_policy as the + # stack's repository reads it now: what the analysis is told of slop, + # the kinds its verdict may name and the heading of a slop finding's + # ticket. None: the section's defaults, for good. + slop_policy: Callable[[], Awaitable[SlopPolicySettings]] | None = None, ) -> None: self._config = config self._signature_masks = signature_masks + self._slop_policy = slop_policy # The masks the last tick clustered with; until one has, the stack's # masks at startup. self._masks: SignatureMasks = _compiled_masks( @@ -1317,13 +1329,24 @@ def _bypasses_cap(self, proposal: dict[str, Any]) -> bool: """ return False + async def _policy(self) -> SlopPolicySettings: + """The stack's slop_policy now (#169); the section's defaults without a reader, or when it fails.""" + if self._slop_policy is None: + return SLOP_POLICY.defaults() + try: + return await self._slop_policy() + except Exception: + logger.warning("watcher_slop_policy_unread", stack=self._repo, exc_info=True) + return SLOP_POLICY.defaults() + async def _recover_proposal(self, job_id: str | None) -> dict[str, Any] | None: """A deferred finding's proposal, re-read from its analysis job. The ledger holds no proposal text (it quotes log lines); the job row does.""" if not job_id: return None job = cast("dict[str, Any] | None", await self._jobs.get(job_id)) - verdict = parse_verdict(str((job or {}).get("result_summary") or "")) + kinds = (await self._policy()).kinds + verdict = parse_verdict(str((job or {}).get("result_summary") or ""), kinds=kinds) return cast("dict[str, Any] | None", verdict["proposal"]) async def _drain_deferred(self, window_days: int, *, slot_free: bool) -> tuple[bool, int]: @@ -1425,9 +1448,13 @@ async def _file_draft( self, proposal: dict[str, Any], job_id: str | None, window_start: datetime, window_end: datetime ) -> dict[str, Any] | None: """Create the DRAFT tracker ticket for a proposal (never labelled).""" - # Slop dungeon: a slop finding's ticket opens by saying so, so whoever - # works it (triage first) is not handed it as a bug. - opening = ticket_section(proposal["work_nature"]) if is_slop(proposal.get("work_nature")) else "" + # Slop dungeon: a slop finding's ticket opens by saying so, under the + # stack's slop_policy.ticket_heading, so whoever works it (triage + # first) is not handed it as a bug. + opening = "" + if is_slop(proposal.get("work_nature")): + heading = (await self._policy()).ticket_heading + opening = ticket_section(proposal["work_nature"], heading=heading) description = ( f"{opening}{proposal['body']}\n\n---\n" f"*Filed by the embry0 watcher (severity: {proposal['severity']}; " @@ -1611,7 +1638,7 @@ async def _analysis_wait(self) -> dict[str, Any] | None: # isinstance, not truthiness: test doubles answer any attribute. return wait.record() if isinstance(wait, AccountWait) else None - async def _analyse(self, task: str) -> dict[str, Any]: + async def _analyse(self, task: str, *, kinds: Collection[str] = DEFAULT_KINDS) -> dict[str, Any]: """Run the analysis to a parsed verdict, re-running it ONCE on garbage. SH-P2.4: a completed job whose deliverable matches neither contract @@ -1646,7 +1673,7 @@ async def _analyse(self, task: str) -> dict[str, Any]: if job and job.get("error_code"): failure += f" error_code={job['error_code']}" break - verdict = parse_verdict(str(job.get("result_summary") or "")) + verdict = parse_verdict(str(job.get("result_summary") or ""), kinds=kinds) if verdict["kind"] != "unparseable": failure = None break @@ -1925,6 +1952,9 @@ async def record(action: str, **fields: Any) -> dict[str, Any]: if clusters and self._clustering_mode() == "on": excerpt_block = format_cluster_table(clusters, redact=redact_log_text) + # #169: what the analysis is told of slop, and the kinds its verdict may name, are the + # stack's repository's slop_policy, read for this analysis. + policy = await self._policy() task = _ANALYSIS_TASK_TEMPLATE.format( window_start=window_start.strftime("%Y-%m-%d %H:%M"), window_end=window_end.strftime("%Y-%m-%d %H:%M"), @@ -1933,7 +1963,7 @@ async def record(action: str, **fields: Any) -> dict[str, Any]: known_issues=known_issues, excerpts=excerpt_block, drilldown=self._drilldown_section(), - slop_guide=CLASSIFIER_GUIDE, + slop_guide=policy.classifier_guide, slop_lines=WATCHER_CONTRACT_LINES, **sections, ) @@ -1944,7 +1974,7 @@ async def record(action: str, **fields: Any) -> dict[str, Any]: return await record( "skipped_account_paused", log_lines=len(lines), detail=_account_wait_detail(waiting) ) - analysis = await self._analyse(task) + analysis = await self._analyse(task, kinds=policy.kinds) tick_fields.update(analysis["run_fields"]) if analysis.get("account_wait"): return await record( diff --git a/embry0/settings/document.py b/embry0/settings/document.py index 739ebcb0..eb28531f 100644 --- a/embry0/settings/document.py +++ b/embry0/settings/document.py @@ -26,7 +26,7 @@ import structlog import yaml -from embry0.settings.layers import cap_of, merged, over_default +from embry0.settings.layers import cap_of, merged, over_default, resolve, settable_values from embry0.settings.registry import ( DOCUMENT_KEYS, MANAGED_BY_FILE, @@ -378,9 +378,12 @@ def check( (:func:`embry0.settings.registry.validate_values`): the settings store passes it on every write it stores, and each section gets it with its values before the write under ``STORED``, so a value the write leaves as - it was is not checked against the deployment again. Offline, and for a - layer's file checked without what it inherits, there is none, and those - checks are left to the write. + it was is not checked against the deployment again. A section's + ``effective_check`` is then given the section as the document's readers + read it before and after the write (:func:`effective_models`), so a value + that changes because the write took its key out is checked too. Offline, + and for a layer's file checked without what it inherits, there is none, + and those checks are left to the write. """ problems: list[Problem] = [] data: dict[str, Any] = {} @@ -543,6 +546,19 @@ def check( clean = {**{name: dumped[name] for name in known}, **own, **kept} if clean: data[key] = clean + if context is not None and (layer == "deployment" or inherited is not None): + refused = {str(problem.loc[0]) for problem in problems if problem.loc} + problems.extend( + _effective_problems( + catalog, + stored, + data, + layer=layer, + inherited=inherited if layer != "deployment" else None, + context=context, + skip=refused, + ) + ) if problems: raise DocumentInvalid(problems) return Checked( @@ -553,6 +569,62 @@ def check( ) +def effective_models( + catalog: Catalog, + data: Mapping[str, Any], + *, + layer: str = "deployment", + inherited: Mapping[str, Mapping[str, Any]] | None = None, + scope: str = "", +) -> dict[str, SectionModel]: + """Each section as a reader of the document at ``layer`` holding ``data`` gets it, defaults included. + + ``inherited`` None reads the deployment's document; a layer reads the + values it may set over ``inherited``, what it inherits, without defaults. + ``scope`` names the layer in what a refusal would say. Never raises. + """ + if inherited is None: + return dict(read_sections(catalog, data).models) + out: dict[str, SectionModel] = {} + for section in catalog: + base = dict(inherited.get(section.name) or {}) + values = settable_values(section, layer, data) + out[section.name] = resolve(section, base, [(scope or layer, values)] if values else []).model + return out + + +def _effective_problems( + catalog: Catalog, + before: Mapping[str, Any], + after: Mapping[str, Any], + *, + layer: str, + inherited: Mapping[str, Mapping[str, Any]] | None, + context: Mapping[str, Any], + skip: set[str], +) -> list[Problem]: + """What each section's ``effective_check`` refuses of a write, from its readers' values before and after it. + + A section refused already (``skip``) is left out: what it would hold is + not known. + """ + checks = { + section.name: section.effective_check + for section in catalog + if section.effective_check is not None and section.name not in skip + } + if not checks: + return [] + sections = Catalog([section for section in catalog if section.name in checks]) + old = effective_models(sections, before, layer=layer, inherited=inherited) + new = effective_models(sections, after, layer=layer, inherited=inherited) + problems: list[Problem] = [] + for name, check_effective in checks.items(): + for key, why in check_effective(old[name], new[name], context): + problems.append(Problem((name, key), "unavailable", why)) + return problems + + def _patch_problems( catalog: Catalog, name: str, keys: Mapping[str, Any], *, held: tuple[Mapping[str, Any], ...], layer: str ) -> list[Problem]: diff --git a/embry0/settings/example.py b/embry0/settings/example.py index 8116d847..46eeabb4 100644 --- a/embry0/settings/example.py +++ b/embry0/settings/example.py @@ -82,10 +82,18 @@ def _section(section: Section[Any], *, commented: bool) -> list[str]: lines.append("") lines.extend(_comment(section.model.model_fields[key].description or "", indent)) mark = "# " if commented else "" - lines.extend(f"{indent}{mark}{line}" for line in dump_yaml({key: defaults[key]}).splitlines()) + lines.extend(_key_line(indent, mark, line) for line in dump_yaml({key: defaults[key]}).splitlines()) return lines +def _key_line(indent: str, mark: str, line: str) -> str: + """One line of a key and its value at the section's indent, commented with ``mark``. A blank line of a + text's block (a guide with paragraphs) is the bare mark, so the file holds no trailing blank.""" + if line: + return f"{indent}{mark}{line}" + return f"{indent}{mark.rstrip()}" + + def render(catalog: Catalog, *, commented: bool = True) -> str: """The example document for ``catalog``: every key commented out, or with ``commented=False`` set.""" lines = [*_comment(_HEADER.rstrip("\n")), f"{VERSION_KEY}: {SCHEMA_VERSION}", ""] diff --git a/embry0/settings/registry.py b/embry0/settings/registry.py index eb0895f0..7281ad5c 100644 --- a/embry0/settings/registry.py +++ b/embry0/settings/registry.py @@ -216,6 +216,11 @@ class EnvImport: relied_on: ReliedOn | None = None +#: ``(before, after, context) -> [(key, why)]``: a section's check of what a write's readers read, the section's +#: model before and after the write, with the write's context (:attr:`Section.effective_check`). +EffectiveCheck = Callable[[Any, Any, Mapping[str, Any]], Iterable[tuple[str, str]]] + + @dataclass(frozen=True) class Section[M: SectionModel]: """One feature's part of the settings document. @@ -239,7 +244,14 @@ class Section[M: SectionModel]: ``env`` maps a field to the ``Embry0Config`` attribute its value is imported from, once (``embry0.settings.bootstrap``): a string is the attribute, an :class:`EnvImport` adds a converter, the parts it drops, or a - :class:`ReliedOn`. + :class:`ReliedOn`. ``effective_check`` is for a value a write is checked + against the running deployment by what its readers read, not by what the + write sets: it is given the section as the written document's readers + read it before the write and after it, defaults included, and the write's + context (:func:`validate_values`), and names each key whose new value the + deployment cannot take, with why. A field's own validator never sees a + key a write takes out, whose value then falls back to what the document + inherits or to its default (:data:`EffectiveCheck`). """ name: str @@ -250,6 +262,7 @@ class Section[M: SectionModel]: env: Mapping[str, str | EnvImport] = field(default_factory=dict) merged_keys: frozenset[str] = frozenset() added_keys: frozenset[str] = frozenset() + effective_check: EffectiveCheck | None = None def __post_init__(self) -> None: object.__setattr__(self, "org_keys", frozenset(self.org_keys)) diff --git a/embry0/settings/sections/__init__.py b/embry0/settings/sections/__init__.py index a182fa5f..f6ba6b6a 100644 --- a/embry0/settings/sections/__init__.py +++ b/embry0/settings/sections/__init__.py @@ -25,6 +25,7 @@ from embry0.settings.sections.salvage import SALVAGE from embry0.settings.sections.scout import SCOUT from embry0.settings.sections.slop_meter import SLOP_METER +from embry0.settings.sections.slop_policy import SLOP_POLICY from embry0.settings.sections.ui import UI CATALOG: Catalog = Catalog( @@ -39,6 +40,7 @@ SALVAGE, SCOUT, SLOP_METER, + SLOP_POLICY, UI, ] ) diff --git a/embry0/settings/sections/slop_policy.py b/embry0/settings/sections/slop_policy.py new file mode 100644 index 00000000..99c649ce --- /dev/null +++ b/embry0/settings/sections/slop_policy.py @@ -0,0 +1,332 @@ +"""The ``slop_policy`` section: what embry0 does with a code job or a finding judged slop, not a bug. + +An operator writes it the way they would say it:: + + slop_policy: + route_template: tidy-up # the pipeline a code job judged slop runs + min_score_to_route: 60 # only a verdict this sure of it is routed + ticket_heading: Shape, not behaviour + + slop_policy: # PUT /config/settings?owner=acme: acme's jobs keep their route + enabled: false + +Triage and the watcher classify a code job or a finding as a bug, slop or +noise (spec 2026-10-25, slop dungeon). What that verdict sets in motion is +here: whether and where a code job judged slop is routed, what the +classifiers and the reviewer of a cleanup are told, the kinds a verdict may +name, the heading a slop finding's ticket opens with, and the Slop-O-Meter +levels at which triage leans toward slop and an area has a Slop Dungeon. +Every default is what the code held before this section, so a deployment +that writes nothing routes, prompts and files exactly as it did. + +**Layers.** Whether, where and how surely a code job is routed are an +organization's and a repository's to say, a repository's layer over its +organization's (the rule of the ``background`` section, #165): a person can +already name the pipeline of any job of a repository. The rest is an +organization's: it is the organization's own definition of slop, the bar +its cleanups are reviewed against and how its tickets read. Those words go +into agents' system prompts, so a repository's layer, which a key with a +repository allowlist may write, holds none of them. + +**When it is read.** A job reads this section for its repository once, when +it starts, and keeps it to its end (:mod:`embry0.services.job_settings`); +the watcher reads its stack's repository's for each analysis and each +draft it files, and the world its repositories' each time it draws them. + +**The route template** is checked whenever a write changes the template a +document's readers route to (:func:`route_template_changed`, through +:data:`ROUTE_TEMPLATES`): by naming another, or by taking the key out, so the +document reads what it inherits or the default. It must name a pipeline +template that takes a code job +(``embry0.workflows.row_behaviour.code_job_problem``). While a document +routes to a template, the template cannot be deleted, renamed or changed so +that it no longer takes one (409, ``embry0.services.slop_routing``). One that +is gone anyway is logged and the job takes the route its kind defaults to. + +**Experience** for a merged cleanup is the progression pack's +``outcomes.cleaned`` (#176), already an operator's document: it is not a key +here. No environment variable fed any of these keys: there is nothing to +import. +""" + +from __future__ import annotations + +import re +import unicodedata +from collections.abc import Callable, Mapping +from typing import Annotated, Any, Final + +from pydantic import Field, StringConstraints, ValidationInfo, field_validator + +from embry0.settings.registry import STORED, Section, SectionModel, refuse_unprintable +from embry0.settings.sections.slop_meter import LEVELS + +# ---- the defaults: what the code held before this section ---- + +DEFAULT_ROUTE_TEMPLATE: Final = "slop-cleanup" +DEFAULT_KINDS: Final = ( + "duplication", + "dead_code", + "half_wired", + "contradicting_layers", + "symptom_patch", + "misleading_docs", + "off_pattern", +) +DEFAULT_CLASSIFIER_GUIDE: Final = """\ +Tell slop apart from bugs: +- bug: the behavior differs from what the code, its docs or the ticket intend. + Fixing it changes behavior. +- slop: the behavior is what the code says, and the trouble is the code's + shape. Cleaning it keeps behavior. Kinds: duplication (the same logic in + more than one place), dead_code (paths, flags or files nothing reaches), + half_wired (a feature or abstraction started and not finished), + contradicting_layers (two layers that disagree about one thing), + symptom_patch (guards, retries, fallbacks or catch-alls stacked on a symptom + instead of its cause), misleading_docs (comments or docs that say what the + code does not do), off_pattern (code that ignores the repository's own way). +- noise: nothing to do. +A guard, retry or catch-all stacked on a symptom is slop when removing it and +fixing the shape keeps the outcome. When the code does the wrong thing (keeps +retrying what can never succeed, drops or duplicates data, answers wrongly), +it is a bug, even when sloppy code caused it: say bug, and name the slop in +the evidence. Do not call a real defect slop to make it look smaller. +slop_score (0-100) is how much of the trouble is shape rather than behavior: +0 for a pure bug, 100 for pure slop.""" +DEFAULT_REVIEW_GUIDE: Final = """\ +## This change is a slop cleanup + +It was judged slop, not a bug: it must keep behavior and improve shape. +Approve only when all of these hold, and request changes otherwise, saying which: +- behavior is unchanged: the tests pass, and no public behavior, API or output + moved (removing what nothing reaches is fine); +- the change removes or merges more than it adds, or is plainly simpler; +- it adds no new guard, retry, fallback, catch-all or flag to hide a symptom.""" +DEFAULT_TICKET_HEADING: Final = "Slop, not a bug" +#: Sloppy Joe, the fourth of the meter's five levels by default. +DEFAULT_TRIAGE_LEAN_MIN_LEVEL: Final = 4 +#: Spaghetti Bolognese, the second. +DEFAULT_DUNGEON_MIN_LEVEL: Final = 2 + +# ---- bounds ---- + +#: The longest template name ``pipeline_templates.name`` holds. +MAX_TEMPLATE_NAME_LENGTH: Final = 100 +MAX_KINDS: Final = 20 +MAX_KIND_LENGTH: Final = 40 +#: A kind as a verdict names it once read: lowercase words joined by ``_`` (``dead_code``). +KIND: Final = re.compile(r"[a-z][a-z0-9_]*") +MAX_CLASSIFIER_GUIDE_LENGTH: Final = 4000 +MAX_REVIEW_GUIDE_LENGTH: Final = 2000 +MAX_TICKET_HEADING_LENGTH: Final = 80 + +#: The validation context key under which a write passes the route template check: ``name -> problem or +#: None``, built from the deployment's pipeline templates (``embry0.settings.write_context``). +ROUTE_TEMPLATES: Final = "route_templates" + +Kind = Annotated[str, StringConstraints(max_length=MAX_KIND_LENGTH)] +Checker = Callable[[str], str | None] + + +def _plain_text(value: str, what: str) -> str: + """``value`` when it is text an agent can be told: lines and tabs, and something besides blanks.""" + if not value.strip(): + raise ValueError(f"write what {what}, not blanks") + if any( + unicodedata.category(char) in ("Cf", "Zl", "Zp") or (unicodedata.category(char) == "Cc" and char not in "\n\t") + for char in value + ): + raise ValueError("the text is plain: lines and tabs, no control, format or separator character") + return value + + +class SlopPolicySettings(SectionModel): + # ---- routing ---- + enabled: bool = Field( + default=True, + description=( + "A code job judged slop runs route_template instead of the route triage picked. False: it takes its " + "normal route, the verdict is still recorded on the job, and triage is told so." + ), + ) + route_template: str = Field( + default=DEFAULT_ROUTE_TEMPLATE, + min_length=1, + max_length=MAX_TEMPLATE_NAME_LENGTH, + description=( + "The pipeline template a code job judged slop runs. A write that changes it, by naming another or by " + "taking the key out, must leave a template that takes a code job: one whose jobs are recorded as code " + "jobs and whose route has a coding step. A template a document routes to cannot be deleted, renamed " + "or changed to stop taking one. A job pinned to the full pipeline takes it only when its route also " + "has review and QA. A template a person named for the job still wins." + ), + ) + min_score_to_route: int = Field( + default=0, + ge=0, + le=100, + description=( + "The least slop_score (0 to 100, how much of the trouble is shape) a slop verdict needs to be " + "routed. 0 routes every slop verdict; one below it takes its normal route." + ), + ) + # ---- what the classifiers are told ---- + kinds: tuple[Kind, ...] = Field( + default=DEFAULT_KINDS, + max_length=MAX_KINDS, + description=( + "The kinds of slop a verdict may name, as lowercase words joined by _. A kind a classifier names " + "that is not here is dropped from its verdict. Describe each in classifier_guide, which is what the " + "classifiers are told." + ), + ) + classifier_guide: str = Field( + default=DEFAULT_CLASSIFIER_GUIDE, + max_length=MAX_CLASSIFIER_GUIDE_LENGTH, + description=( + "What triage and the watcher are told slop, a bug and noise are, and what slop_score means, in " + "their system prompts. Name each of kinds in it." + ), + ) + review_guide: str = Field( + default=DEFAULT_REVIEW_GUIDE, + max_length=MAX_REVIEW_GUIDE_LENGTH, + description=( + "What the reviewer of a change the Slopmaster made is told to check, before its usual instructions. " + "The lines the branch adds and removes are said after it." + ), + ) + ticket_heading: str = Field( + default=DEFAULT_TICKET_HEADING, + max_length=MAX_TICKET_HEADING_LENGTH, + description=( + "The heading, without its # marks, that the watcher's draft ticket for a finding judged slop opens " + "with, in the tracker it files to. Triage is told to weigh a section under it." + ), + ) + # ---- what the Slop-O-Meter's reading does ---- + triage_lean_min_level: int | None = Field( + default=DEFAULT_TRIAGE_LEAN_MIN_LEVEL, + ge=1, + le=LEVELS, + description=( + "The Slop-O-Meter level (1 to 5) from which triage leans toward slop on a borderline verdict, told " + "with the levels' names (slop_meter.level_names). Null: it is given the reading and never told to " + "lean." + ), + ) + dungeon_min_level: int | None = Field( + default=DEFAULT_DUNGEON_MIN_LEVEL, + ge=1, + le=LEVELS, + description=( + "The Slop-O-Meter level (1 to 5) from which a repository or an area has a Slop Dungeon on the world " + "map even with no open slop finding. Null: only open slop and recent cleanups make one." + ), + ) + + @field_validator("route_template") + @classmethod + def _template_name(cls, value: str) -> str: + if value != value.strip(): + raise ValueError("write the template's name as it is, with no blanks around it") + refuse_unprintable(value, what="a template's name") + return value + + @field_validator("route_template") + @classmethod + def _template_offered(cls, value: str, info: ValidationInfo) -> str: + """On a write that changes the template its readers route to: one this deployment has that takes a code + job. The value a document reads before the write is its own or what it inherits, else the default.""" + context = info.context if isinstance(info.context, Mapping) else {} + checker: Checker | None = context.get(ROUTE_TEMPLATES) + if checker is None: + return value + before = context.get(STORED) + if isinstance(before, Mapping) and before.get("route_template", DEFAULT_ROUTE_TEMPLATE) == value: + # The template its readers routed to already: never judged again by a write of another key. + return value + problem = checker(value) + if problem: + raise ValueError(problem) + return value + + @field_validator("kinds") + @classmethod + def _kinds(cls, value: tuple[str, ...]) -> tuple[str, ...]: + if not value: + raise ValueError("name at least one kind: a slop verdict names the kinds of its slop") + for position, kind in enumerate(value, start=1): + if KIND.fullmatch(kind) is None: + raise ValueError( + f"kind {position}: write lowercase letters, digits and _, starting with a letter (dead_code)" + ) + if len(set(value)) != len(value): + raise ValueError("name each kind once") + return value + + @field_validator("classifier_guide") + @classmethod + def _classifier_guide(cls, value: str) -> str: + return _plain_text(value, "the classifiers are told") + + @field_validator("review_guide") + @classmethod + def _review_guide(cls, value: str) -> str: + return _plain_text(value, "the reviewer is told") + + @field_validator("ticket_heading") + @classmethod + def _ticket_heading(cls, value: str) -> str: + words = value.strip() + if not words: + raise ValueError("write the heading's words") + refuse_unprintable(words, what="a heading") + if words.startswith("#"): + raise ValueError("write the heading's words without its # marks, such as Slop, not a bug") + return words + + +def route_template_changed( + before: SlopPolicySettings, after: SlopPolicySettings, context: Mapping[str, Any] +) -> list[tuple[str, str]]: + """On a write that leaves a document's readers on the default template, when they routed elsewhere before: + the default must be a template this deployment has that takes a code job. + + ``before`` and ``after`` are the section as the written document's + readers read it, defaults included. A value a document of the chain + sets is checked by the field's own validator as the write is checked; + the default is not, so a write that takes ``route_template`` out of the + last document that set it, or the whole section, is checked here. A + template the write leaves as it was is never judged again: a write of + another key goes through while the template its readers route to is + missing, and a job then takes its kind's default. + """ + checker: Checker | None = context.get(ROUTE_TEMPLATES) + if checker is None or "route_template" in after.model_fields_set: + return [] + if after.route_template == before.route_template: + return [] + problem = checker(after.route_template) + return [("route_template", problem)] if problem else [] + + +#: What decides a job's route: an organization's, and a repository's for its own jobs. +_ROUTE_KEYS: Final = frozenset({"enabled", "route_template", "min_score_to_route"}) + +SLOP_POLICY = Section( + name="slop_policy", + model=SlopPolicySettings, + summary=( + "What embry0 does with a code job or a finding judged slop: whether and where it is routed, what the " + "classifiers and the reviewer are told, its kinds, its ticket heading and the meter levels it acts on." + ), + # Routing is per repository, with an organization's default in between, + # as background features are (#165). The words the agents are told, the + # kinds, the heading and the levels are the organization's definition of + # slop: a repository's layer, which a repository-scoped key may write, + # never sets text in a system prompt. + org_keys=frozenset(SlopPolicySettings.model_fields), + repo_keys=_ROUTE_KEYS, + effective_check=route_template_changed, +) diff --git a/embry0/settings/store.py b/embry0/settings/store.py index 543cae39..c24c9783 100644 --- a/embry0/settings/store.py +++ b/embry0/settings/store.py @@ -24,7 +24,9 @@ **Writing.** Every write locks its document, checks the whole document (:func:`embry0.settings.document.check`) with the store's write context (:mod:`embry0.settings.write_context`: what the deployment offers, such as -its model catalogue), a layer together with the values +its model catalogue and, for a write that may change ``slop_policy``, its +pipeline templates, read on the write's own connection), a layer together +with the values it inherits, read under the locks of the documents it inherits from so a concurrent write to them commits first, stores the text and its values, and appends a @@ -58,6 +60,7 @@ changes, check, dump_yaml, + effective_models, load_yaml, patch_section, read_sections, @@ -66,7 +69,7 @@ from embry0.settings.layers import LayerRefusal, Resolved, resolve, settable_values from embry0.settings.registry import DOCUMENT_KEYS, MANAGED_BY_KEY, SCHEMA_VERSION, Catalog, Section, SectionModel from embry0.settings.scope import DEPLOYMENT, Scope, ScopeError, current, organization_of, stored_rows -from embry0.settings.write_context import WriteContext, deployment_context +from embry0.settings.write_context import WriteContext, WriteReads, deployment_context, deployment_reads from embry0.storage import repo_renames from embry0.storage.database import DatabasePool from embry0.storage.repositories.config_revisions import ConfigRevisionsRepository, RevisionSource @@ -479,10 +482,21 @@ class Preview: managed_by: str | None = None -#: What a write makes of the document it replaces: from the stored text, the stored values and what the -#: document inherits (None for the deployment's), the text to store and its checked values. Raises -#: :class:`DocumentInvalid` listing every problem. -Proposal = Callable[[str, Mapping[str, Any], Mapping[str, Mapping[str, Any]] | None], tuple[str, Checked]] +#: What a write makes of the document it replaces: from the stored text, the stored values, what the +#: document inherits (None for the deployment's) and the write's context, the text to store and its checked +#: values. Raises :class:`DocumentInvalid` listing every problem. +Proposal = Callable[ + [str, Mapping[str, Any], Mapping[str, Mapping[str, Any]] | None, Mapping[str, Any]], tuple[str, Checked] +] +#: The sections a write may change, from the stored document's values: what its context reads from the database +#: for (:data:`embry0.settings.write_context.WriteReads`). +Touches = Callable[[Mapping[str, Any]], frozenset[str]] + + +def changed_sections(proposed: Mapping[str, Any], stored: Mapping[str, Any]) -> frozenset[str]: + """The sections ``proposed`` holds otherwise than ``stored`` does: those a write of it may change.""" + names = (set(proposed) | set(stored)) - DOCUMENT_KEYS + return frozenset(str(name) for name in names if proposed.get(name) != stored.get(name)) def _matches(expected: int | Collection[int] | None, generation: int) -> bool: @@ -528,6 +542,7 @@ def __init__( catalog: Catalog | None = None, *, write_context: WriteContext | None = None, + write_reads: WriteReads | None = None, started: datetime | None = None, ) -> None: if catalog is None: @@ -541,6 +556,8 @@ def __init__( self.started = started #: Built for each write: what the deployment offers now (the model catalogue moves while it runs). self._write_context: WriteContext = write_context if write_context is not None else deployment_context + #: Read on each write's connection: what the deployment's database holds (its pipeline templates). + self._write_reads: WriteReads = write_reads if write_reads is not None else deployment_reads self._documents = SettingsDocumentsRepository(db) self._revisions = ConfigRevisionsRepository(db) self._db = db @@ -606,6 +623,11 @@ async def layers(self) -> list[dict[str, Any]]: # ---- writing ------------------------------------------------------------ + async def _context(self, conn: Any, sections: frozenset[str]) -> dict[str, Any]: + """The context a write that may change ``sections`` is checked with: what the deployment offers now, the + database's part read on ``conn`` (the write's connection inside :meth:`edit`, the pool for a dry run).""" + return {**self._write_context(), **(await self._write_reads(conn, sections))} + @asynccontextmanager async def edit( self, *, actor: str, source: RevisionSource, reason: str = "", scope: Scope | None = None @@ -722,6 +744,7 @@ async def save( scope = scope or Scope.deployment() return await self._write( self._whole(text, values, scope), + lambda stored: changed_sections(values, stored), actor=actor, source=source, reason=reason, @@ -751,6 +774,7 @@ async def set_keys( scope = scope or Scope.deployment() return await self._write( self._keys(section, keys, scope), + lambda _stored: frozenset({section}), actor=actor, source=source, reason=reason, @@ -762,7 +786,10 @@ def _whole(self, text: str | None, values: Mapping[str, Any], scope: Scope) -> P """A write of the whole document: ``values``, kept as ``text`` (or the YAML a JSON object dumps to).""" def propose( - stored_text: str, stored: Mapping[str, Any], inherited: Mapping[str, Mapping[str, Any]] | None + stored_text: str, + stored: Mapping[str, Any], + inherited: Mapping[str, Mapping[str, Any]] | None, + context: Mapping[str, Any], ) -> tuple[str, Checked]: checked = check( self._catalog, @@ -771,7 +798,7 @@ def propose( stored_text=stored_text, layer=scope.layer, inherited=inherited, - context=self._write_context(), + context=context, ) return (text if text is not None else dump_yaml(values)), checked @@ -781,7 +808,10 @@ def _keys(self, section: str, keys: Mapping[str, Any], scope: Scope) -> Proposal """A write of some keys of one section, into the stored text.""" def propose( - stored_text: str, stored: Mapping[str, Any], inherited: Mapping[str, Mapping[str, Any]] | None + stored_text: str, + stored: Mapping[str, Any], + inherited: Mapping[str, Mapping[str, Any]] | None, + context: Mapping[str, Any], ) -> tuple[str, Checked]: return patch_section( self._catalog, @@ -791,7 +821,7 @@ def propose( stored_text=stored_text, layer=scope.layer, inherited=inherited, - context=self._write_context(), + context=context, ) return propose @@ -799,6 +829,7 @@ def propose( async def _write( self, propose: Proposal, + touches: Touches, *, actor: str, source: RevisionSource, @@ -812,7 +843,8 @@ async def _write( raise GenerationMismatch(draft.generation) before = draft.data inherited = await self._inherited_now(draft.conn, scope) - new_text, checked = propose(draft.yaml_text, draft.data, inherited) + context = await self._context(draft.conn, touches(draft.data)) + new_text, checked = propose(draft.yaml_text, draft.data, inherited, context) if new_text != draft.yaml_text: draft.replace(new_text, checked.data, checked.schema_version) refused: tuple[list[dict[str, Any]], int] = ([], 0) @@ -839,7 +871,12 @@ async def preview( ) -> Preview: """What :meth:`save` would change, storing nothing. Raises as :meth:`save` does, ``If-Match`` included.""" scope = scope or Scope.deployment() - return await self._dry_run(self._whole(text, values, scope), if_generation=if_generation, scope=scope) + return await self._dry_run( + self._whole(text, values, scope), + lambda stored: changed_sections(values, stored), + if_generation=if_generation, + scope=scope, + ) async def preview_keys( self, @@ -851,10 +888,15 @@ async def preview_keys( ) -> Preview: """What :meth:`set_keys` would change, storing nothing. Raises as it does, ``If-Match`` included.""" scope = scope or Scope.deployment() - return await self._dry_run(self._keys(section, keys, scope), if_generation=if_generation, scope=scope) + return await self._dry_run( + self._keys(section, keys, scope), + lambda _stored: frozenset({section}), + if_generation=if_generation, + scope=scope, + ) async def _dry_run( - self, propose: Proposal, *, if_generation: int | Collection[int] | None, scope: Scope + self, propose: Proposal, touches: Touches, *, if_generation: int | Collection[int] | None, scope: Scope ) -> Preview: """What ``propose`` would make of the stored document, storing nothing.""" snapshot = await self.snapshot() @@ -863,7 +905,8 @@ async def _dry_run( row = await self._documents.get(str(stored)) or _empty(stored) if not _matches(if_generation, int(row["generation"])): raise GenerationMismatch(int(row["generation"])) - new_text, checked = propose(row["yaml_text"], row["data"], inherited) + context = await self._context(self._db, touches(row["data"])) + new_text, checked = propose(row["yaml_text"], row["data"], inherited, context) would_change = new_text != row["yaml_text"] refused: tuple[list[dict[str, Any]], int] = ([], 0) if would_change and scope.layer != "repository": @@ -927,14 +970,16 @@ async def undo(self, revision_id: int, *, actor: str) -> SaveResult: text = str(before_doc.get("yaml_text") or "") problem = Problem((), "revision_invalid", "That earlier version is no longer valid.") try: + restored = load_yaml(text) + context = await self._context(draft.conn, changed_sections(restored, draft.data)) checked = check( self._catalog, - load_yaml(text), + restored, stored=draft.data, stored_text=draft.yaml_text, layer=scope.layer, inherited=inherited, - context=self._write_context(), + context=context, ) except DocumentInvalid as exc: raise RevisionInvalid([problem, *exc.problems]) from None @@ -957,14 +1002,7 @@ def _models( catalog: Catalog, data: Mapping[str, Any], scope: Scope, inherited: Mapping[str, Mapping[str, Any]] | None ) -> dict[str, SectionModel]: """Each section as a reader of the document at ``scope`` holding ``data`` gets it.""" - if inherited is None: - return dict(read_sections(catalog, data).models) - out: dict[str, SectionModel] = {} - for section in catalog: - base = dict(inherited.get(section.name) or {}) - values = settable_values(section, scope.layer, data) - out[section.name] = resolve(section, base, [(str(scope), values)] if values else []).model - return out + return effective_models(catalog, data, layer=scope.layer, inherited=inherited, scope=str(scope)) def describe_changes( diff --git a/embry0/settings/write_context.py b/embry0/settings/write_context.py index 0e5e549b..9fcdb282 100644 --- a/embry0/settings/write_context.py +++ b/embry0/settings/write_context.py @@ -1,14 +1,20 @@ """What a write of the settings document is checked against beyond the values themselves. A section whose values name something the running deployment offers (a -model of its provider catalogue) checks them through pydantic's validation -context, which the settings store builds here for every write it stores: -``PUT`` and ``PATCH /config/settings``, their dry runs, and an undo. A -stored value read back, the environment import at startup and the offline -``embry0 settings check`` validate without it, so what the catalogue offers -moving (OpenRouter's live list is read after startup; a provider is turned -off) never turns a stored value into its default. Such a value is caught -where it is used: the feature logs that its model cannot be called. +model of its provider catalogue, a pipeline template) checks them through +pydantic's validation context, which the settings store builds here for +every write it stores: ``PUT`` and ``PATCH /config/settings``, their dry +runs, and an undo. A stored value read back, the environment import at +startup and the offline ``embry0 settings check`` validate without it, so +what the deployment offers moving (OpenRouter's live list is read after +startup; a provider is turned off) never turns a stored value into its +default. Such a value is caught where it is used: the feature logs that its +model cannot be called, or that its template is gone. + +What lives in the database is read on the write's own connection, inside +its transaction, and only for a write that may change a section that checks +against it (:func:`deployment_reads`); the rest is built in memory +(:func:`deployment_context`). The check lives here, in the orchestrator, and not in ``embry0.agents.providers``, which the sandbox images carry: it reads that @@ -17,13 +23,17 @@ from __future__ import annotations -from collections.abc import Callable, Iterable, Mapping +from collections.abc import Awaitable, Callable, Iterable, Mapping from typing import Any from embry0.settings.sections.background_models import CATALOGUE +from embry0.settings.sections.slop_policy import ROUTE_TEMPLATES, SLOP_POLICY #: Builds the context of one write; the store's default is :func:`deployment_context`. WriteContext = Callable[[], Mapping[str, Any]] +#: ``(conn, sections) -> context``: reads, on one write's connection, the part of its context that lives in the +#: database, for a write that may change ``sections``; the store's default is :func:`deployment_reads`. +WriteReads = Callable[[Any, frozenset[str]], Awaitable[Mapping[str, Any]]] def model_use_problem(model: str, needs: Iterable[str]) -> str | None: @@ -72,3 +82,44 @@ def deployment_context() -> dict[str, Any]: def no_context() -> dict[str, Any]: """No deployment to check against: each value meets only its section's own rules.""" return {} + + +def route_template_checker(rows: Iterable[Mapping[str, Any]]) -> Callable[[str], str | None]: + """``name -> problem``: why a slop policy cannot route a code job to the template ``name``, of ``rows``. + + A template the rows do not hold is none; one they hold must take a code + job (``row_behaviour.code_job_problem``: its jobs recorded as code jobs, + and a coding step in its route). + """ + from embry0.workflows.row_behaviour import code_job_problem + + templates = {str(row["name"]): row for row in rows} + + def problem(name: str) -> str | None: + row = templates.get(name) + if row is None: + return f"no pipeline template named {name!r}: GET /api/v1/pipelines/templates lists them" + return code_job_problem(row) + + return problem + + +async def deployment_reads(conn: Any, sections: frozenset[str]) -> dict[str, Any]: + """The part of a write's context the database holds: the pipeline templates, for a write that may change + ``slop_policy``; nothing for any other. + + ``conn`` is the write's connection, inside its transaction, and the rows + are read ``FOR SHARE``: a template a policy is checked against cannot be + deleted or renamed before the write commits. A change of a template takes + its row ``FOR UPDATE`` before it reads the settings documents + (``embry0.services.slop_routing``), so of a write naming a template and a + change of that template, one waits for the other and sees what it wrote. + A write of another section takes no lock. A dry run reads on the pool, + where the lock ends with the statement. + """ + if SLOP_POLICY.name not in sections: + return {} + rows = await conn.fetch( + "SELECT name, default_for_kind, job_kind, graph_definition FROM pipeline_templates FOR SHARE" + ) + return {ROUTE_TEMPLATES: route_template_checker([dict(row) for row in rows])} diff --git a/embry0/storage/repositories/pipeline_templates.py b/embry0/storage/repositories/pipeline_templates.py index 6ac73c2a..359b4696 100644 --- a/embry0/storage/repositories/pipeline_templates.py +++ b/embry0/storage/repositories/pipeline_templates.py @@ -1,9 +1,10 @@ """Pipeline templates repository — CRUD + duplicate for graph-based pipeline definitions.""" import uuid -from collections.abc import Callable +from collections.abc import Awaitable, Callable, Mapping from typing import Any +import asyncpg import structlog from embry0.storage.database import DatabasePool @@ -33,6 +34,11 @@ } +#: Called inside a template's change once its row is locked ``FOR UPDATE``: ``(conn, before, after)``, ``after`` +#: None for a delete. It raises to refuse the change, which is rolled back (``embry0.services.slop_routing``). +Guard = Callable[[asyncpg.Connection, Mapping[str, Any], Mapping[str, Any] | None], Awaitable[None]] + + def _behaviour(fields: dict[str, Any]) -> dict[str, Any]: """Each behaviour column ``fields`` names, checked: a malformed value raises ValueError naming its field.""" return {key: _CHECKS[key](fields[key]) for key in BEHAVIOUR_FIELDS if key in fields} @@ -105,12 +111,14 @@ async def create( row = await self.get(template_id) return row # type: ignore[return-value] - async def update(self, template_id: str, **fields: Any) -> dict[str, Any]: + async def update(self, template_id: str, *, guard: Guard | None = None, **fields: Any) -> dict[str, Any]: """Update allowed fields on a pipeline template and return the updated record. A malformed behaviour column raises ValueError naming its field. A rename moves each agent's ``quest_template`` that named the old name - to the new one, in the same transaction. + to the new one, in the same transaction. ``guard`` sees the row + before and after, in that transaction, and refuses the change by + raising (:data:`Guard`). """ valid = {k: v for k, v in fields.items() if k in _UPDATABLE_FIELDS} valid.update(_behaviour(valid)) @@ -125,11 +133,14 @@ async def update(self, template_id: str, **fields: Any) -> dict[str, Any]: sets.append("updated_at = NOW()") async with self._db.transaction() as conn: # FOR UPDATE: a rename running beside this one waits for it, then reads the name it left, so - # the agents' tasks follow every rename to the last. - old_name = await conn.fetchval( - "SELECT name FROM pipeline_templates WHERE id = $1 FOR UPDATE", template_id - ) + # the agents' tasks follow every rename to the last; a settings write that reads the templates + # FOR SHARE waits for it too, so the guard and that write see each other's commit. + before = await conn.fetchrow("SELECT * FROM pipeline_templates WHERE id = $1 FOR UPDATE", template_id) + old_name = before["name"] if before is not None else None await conn.execute(f"UPDATE pipeline_templates SET {', '.join(sets)} WHERE id = $1", *args) + if guard is not None and before is not None: + after = await conn.fetchrow("SELECT * FROM pipeline_templates WHERE id = $1", template_id) + await guard(conn, dict(before), dict(after) if after is not None else None) if "name" in valid and old_name is not None and valid["name"] != old_name: # An agent's guild task names its template (#167): it follows a rename, in the same # transaction, so the task keeps running what the operator chose. @@ -143,9 +154,13 @@ async def update(self, template_id: str, **fields: Any) -> dict[str, Any]: row = await self.get(template_id) return row # type: ignore[return-value] - async def delete(self, template_id: str) -> None: - """Delete a pipeline template by id.""" - await self._db.execute("DELETE FROM pipeline_templates WHERE id = $1", template_id) + async def delete(self, template_id: str, *, guard: Guard | None = None) -> None: + """Delete a pipeline template by id; ``guard`` sees its row, locked, and refuses by raising (:data:`Guard`).""" + async with self._db.transaction() as conn: + before = await conn.fetchrow("SELECT * FROM pipeline_templates WHERE id = $1 FOR UPDATE", template_id) + if guard is not None and before is not None: + await guard(conn, dict(before), None) + await conn.execute("DELETE FROM pipeline_templates WHERE id = $1", template_id) logger.info("pipeline_template_deleted", template_id=template_id) async def upsert_builtin( diff --git a/embry0/storage/repositories/settings_documents.py b/embry0/storage/repositories/settings_documents.py index be18282f..e64ac402 100644 --- a/embry0/storage/repositories/settings_documents.py +++ b/embry0/storage/repositories/settings_documents.py @@ -51,13 +51,14 @@ async def token(self) -> tuple[int, int, datetime | None]: ) return token_of(row) - async def all(self) -> list[dict[str, Any]]: + async def all(self, conn: asyncpg.Connection | None = None) -> list[dict[str, Any]]: """Every document's values and import ledger in one statement, so a snapshot's rows belong together; - never the text.""" - rows = await self._db.fetch( + never the text. On ``conn`` when given: inside the caller's transaction.""" + sql = ( "SELECT scope, data, schema_version, generation, bootstrapped_keys, updated_by, updated_at " "FROM settings_documents" ) + rows = await (conn.fetch(sql) if conn is not None else self._db.fetch(sql)) out = [] for record in rows: row = dict(record) diff --git a/embry0/workflows/issue_to_pr/nodes/_review.py b/embry0/workflows/issue_to_pr/nodes/_review.py index 45fd2cc8..887dff28 100644 --- a/embry0/workflows/issue_to_pr/nodes/_review.py +++ b/embry0/workflows/issue_to_pr/nodes/_review.py @@ -148,14 +148,17 @@ def _configured_base_branch(state: dict[str, Any]) -> str | None: def _slop_review_section(state: dict[str, Any]) -> str: """What the reviewer checks of a slop cleanup (spec 2026-10-25, slop dungeon); '' for any other change. - Placed before the JSON contract, so a resumed review's delta prompt - (which keeps only the contract) does not resend it. + The job's ``slop_policy.review_guide``, as it read the section when it + started (#169). Placed before the JSON contract, so a resumed review's + delta prompt (which keeps only the contract) does not resend it. """ - from embry0.services.slop import SLOP_REVIEW_GUIDE, SLOPMASTER + from embry0.services.job_settings import job_section + from embry0.services.slop import SLOPMASTER + from embry0.settings.sections.slop_policy import SLOP_POLICY if state.get("coding_agent") != SLOPMASTER: return "" - section = SLOP_REVIEW_GUIDE + section = job_section(state, SLOP_POLICY).review_guide cleanup = (state.get("work_nature") or {}).get("cleanup") if isinstance(cleanup, dict) and {"added", "removed"} <= cleanup.keys(): section += f"\nThe branch adds {cleanup['added']} lines and removes {cleanup['removed']} against its base." diff --git a/embry0/workflows/issue_to_pr/nodes/_triage.py b/embry0/workflows/issue_to_pr/nodes/_triage.py index 1914e32e..078eeb40 100644 --- a/embry0/workflows/issue_to_pr/nodes/_triage.py +++ b/embry0/workflows/issue_to_pr/nodes/_triage.py @@ -226,12 +226,14 @@ async def triage_node(state: dict[str, Any], config: RunnableConfig) -> dict[str prompt += f"\n\n{open_prs_block}" # The Slop-O-Meter (spec 2026-10-26): how sloppy the job's area is, so a - # borderline bug-or-slop verdict leans toward slop in a sloppy one. - from embry0.services.slop_meter.context import job_meter, meter_sentence + # borderline bug-or-slop verdict leans toward slop in a sloppy one, from + # the level its slop_policy names (#169), told by the meter's own names. + from embry0.services.slop_meter.context import job_meter, level_names, meter_sentence reading = await job_meter(config, state) if reading is not None: prompt += f"\n\n{meter_sentence(*reading)}" + meter_levels = await level_names(config, repo) # EMB-35: the triage instructions ride as a real system prompt (appended # to the CLI's preset system block, which its prompt caching covers) @@ -253,9 +255,12 @@ async def triage_node(state: dict[str, Any], config: RunnableConfig) -> dict[str # those settings change, so it still caches. default_model, tier_models = await load_triage_model_settings(config) # #182: the "Done when" section names the ticket headings the job's - # repository writes its criteria under, as the job read them at its start. + # repository writes its criteria under, as the job read them at its start; + # #169: "Bug or slop" says what its slop_policy tells the classifiers and + # does to the route. from embry0.services.job_settings import job_section from embry0.settings.sections.done_when import DONE_WHEN + from embry0.settings.sections.slop_policy import SLOP_POLICY # The skill selection offers the skills the developer's row lets triage # add, from a source its skills load; none, or no row, leaves it out. @@ -264,6 +269,8 @@ async def triage_node(state: dict[str, Any], config: RunnableConfig) -> dict[str default_model=default_model, tier_models=tier_models, ticket_sections=job_section(state, DONE_WHEN).ticket_sections, + slop_policy=job_section(state, SLOP_POLICY), + level_names=meter_levels, skill_choices=triage_skill_choices(developer_row or {}), ) diff --git a/embry0/workflows/issue_to_pr/plan_route.py b/embry0/workflows/issue_to_pr/plan_route.py index 68e48892..ba66dee9 100644 --- a/embry0/workflows/issue_to_pr/plan_route.py +++ b/embry0/workflows/issue_to_pr/plan_route.py @@ -23,7 +23,7 @@ from langgraph.config import get_stream_writer from embry0.execution import events, sandbox_files -from embry0.services.slop import SLOP_TEMPLATE, is_slop, parse_work_nature +from embry0.services.slop import is_slop, parse_work_nature from embry0.workflows._validation import validate_graph_definition from embry0.workflows.issue_to_pr.route_plan import ( GENERIC_AGENT_TYPES, @@ -32,7 +32,14 @@ coding_agent_of, triage_template_config, ) -from embry0.workflows.row_behaviour import builtin_template, leader, offered_to_triage, recorded_kind +from embry0.workflows.row_behaviour import ( + builtin_template, + code_job_problem, + full_pipeline_problem, + leader, + offered_to_triage, + recorded_kind, +) logger = structlog.get_logger(__name__) @@ -346,13 +353,19 @@ async def plan_route_node(state: dict[str, Any], config: RunnableConfig) -> dict 1. triage's explicit ``pipeline_template`` name (legacy values skipped, and a template triage is not offered, which runs only when the job - names it: ``offered_to_triage``, #167); + names it: ``offered_to_triage``, #167), or, for a code job judged + slop, the template its repository's ``slop_policy`` routes it to + (#169) when the policy routes and the verdict's score reaches its + threshold; 2. the template whose ``default_for_kind`` matches the job_kind; 3. the legacy synthesized plan (bit-for-bit today's dev→review→qa chain). An invalid template falls through to the next tier — except an *explicitly requested* invalid template with no kind default, which - fails loudly (silently-wrong routing would be worse). + fails loudly (silently-wrong routing would be worse). The slop route is + a policy's, not a person's: one that is gone, invalid, no longer takes + a code job, or lacks the review and QA steps a pinned job keeps, is + logged and falls through, and never fails the job. #306: a job working a watcher-origin case (``pin_full_pipeline``) always runs the full code pipeline — the ``code`` kind default (review and QA), @@ -400,18 +413,43 @@ async def plan_route_node(state: dict[str, Any], config: RunnableConfig) -> dict requested, explicit = "", False # Slop dungeon (spec 2026-10-25): a code job triage judged slop, not a - # bug, runs the Slopmaster's cleanup. It keeps review and QA, so a - # watcher-origin job may take it too; a template a person named wins - # below. A missing slop-cleanup template falls to the kind default. - work_nature = parse_work_nature(decision.get("work_nature")) + # bug, runs the cleanup its repository's slop_policy names (#169), as the + # job read it when it started: unless the policy is off for it, or the + # verdict's score is below its threshold. A job pinned to the full + # pipeline (a watcher-origin case, #306) takes it only when its route has + # review and QA steps. A template a person named wins below. A route + # template that is gone, no longer takes a code job, or lacks the steps a + # pinned job keeps, is logged and the job takes the route its kind + # defaults to. + from embry0.services.job_settings import job_section + from embry0.settings.sections.slop_policy import SLOP_POLICY + + policy = job_section(state, SLOP_POLICY) + work_nature = parse_work_nature(decision.get("work_nature"), kinds=policy.kinds) + slop_route: str | None = None if is_slop(work_nature) and job_kind == "code": - logger.info( - "plan_route_slop_cleanup", - job_id=state.get("job_id"), - triage_template=requested or None, - slop_score=(work_nature or {}).get("slop_score"), - ) - requested, explicit = SLOP_TEMPLATE, True + score = int((work_nature or {}).get("slop_score") or 0) + if not policy.enabled or score < policy.min_score_to_route: + logger.info( + "plan_route_slop_not_routed", + job_id=state.get("job_id"), + reason=( + "slop_policy.enabled is false" + if not policy.enabled + else f"its slop score is below slop_policy.min_score_to_route ({policy.min_score_to_route})" + ), + slop_score=score, + ) + else: + logger.info( + "plan_route_slop_cleanup", + job_id=state.get("job_id"), + triage_template=requested or None, + slop_score=score, + template=policy.route_template, + ) + slop_route = policy.route_template + requested, explicit = slop_route, True plan: list[dict[str, Any]] | None = None resolved_name: str | None = None @@ -425,19 +463,28 @@ async def plan_route_node(state: dict[str, Any], config: RunnableConfig) -> dict if explicit_user: candidates.append(("explicit", await _find_template(templates_repo, name=requested_user))) if explicit and requested != requested_user: - candidates.append(("explicit", await _find_template(templates_repo, name=requested))) + tier = "slop" if requested == slop_route else "explicit" + candidates.append((tier, await _find_template(templates_repo, name=requested))) candidates.append(("kind_default", await _find_template(templates_repo, kind=job_kind))) agent_types = await _known_agent_types(config) for tier, row in candidates: if row is None: - if tier == "explicit": + if tier in ("explicit", "slop"): logger.warning( "plan_route_template_not_found", requested=requested_user or requested, job_kind=job_kind, + **({"slop_route": True} if tier == "slop" else {}), ) continue + if tier == "slop": + unfit = code_job_problem(row) + if unfit is None and state.get("pin_full_pipeline"): + unfit = full_pipeline_problem(row) + if unfit is not None: + logger.warning("plan_route_slop_template_unfit", template=row["name"], problem=unfit) + continue graph = row.get("graph_definition") or {} problems = validate_graph_definition(row["name"], graph, agent_types) if problems: @@ -482,7 +529,6 @@ async def plan_route_node(state: dict[str, Any], config: RunnableConfig) -> dict done_when = None if effective_kind == "code": from embry0.services.done_when import person_list, resolve - from embry0.services.job_settings import job_section from embry0.settings.sections.done_when import DONE_WHEN done_when = await person_list(jobs_repo, str(state.get("job_id") or ""), state.get("done_when")) diff --git a/embry0/workflows/row_behaviour.py b/embry0/workflows/row_behaviour.py index 21df6617..7403a259 100644 --- a/embry0/workflows/row_behaviour.py +++ b/embry0/workflows/row_behaviour.py @@ -65,6 +65,10 @@ CODING_AGENT_TYPES: Final[tuple[str, ...]] = ("developer", "slopmaster") #: The coding agent of a route that names no other, and of a job that recorded none. DEFAULT_CODER: Final = "developer" +#: The agent types of a route's review step and of its QA step, the graph's own vocabulary too: a job pinned to +#: the full pipeline (#306) keeps both whatever route a policy sends it to (:func:`full_pipeline_problem`). +REVIEW_AGENT_TYPES: Final[tuple[str, ...]] = ("reviewer", "review") +QA_AGENT_TYPE: Final = "qa" #: Each column #167 added to ``pipeline_templates``, with what a row without it means. TEMPLATE_DEFAULTS: Final[Mapping[str, Any]] = MappingProxyType( @@ -138,6 +142,47 @@ def leader(template: Mapping[str, Any] | None) -> str | None: return str(_column(template, "leader_agent", TEMPLATE_DEFAULTS) or "") or None +def code_job_problem(template: Mapping[str, Any]) -> str | None: + """Why a code job cannot be sent to ``template`` by a policy (``slop_policy.route_template``); None when it can. + + The row says how a job of the template is recorded (:func:`recorded_kind`): + one recorded as another kind, the research default or a route of + specialists, would not run a code job as one. And its route needs a + coding step (:data:`CODING_AGENT_TYPES`), or the job would change + nothing. The words name the template, never a value of another key. + """ + name = str(template.get("name") or "") + kind = recorded_kind(template) + if kind is not None and kind != "code": + return f"{name!r} records its jobs as {kind} jobs, so a code job sent to it would not run as one" + nodes = (template.get("graph_definition") or {}).get("nodes") or [] + if not any(str(node.get("agent_type") or "") in CODING_AGENT_TYPES for node in nodes): + return ( + f"{name!r} has no coding step ({', '.join(CODING_AGENT_TYPES)}), so a code job sent to it would change " + "nothing" + ) + return None + + +def full_pipeline_problem(template: Mapping[str, Any]) -> str | None: + """Why a job pinned to the full pipeline cannot be sent to ``template`` by a policy; None when it can. + + A job working a watcher-origin case keeps review and QA (#306, + ``pin_full_pipeline``): a route a policy sends it to must have a review + step (:data:`REVIEW_AGENT_TYPES`) and a QA step (:data:`QA_AGENT_TYPE`). + """ + name = str(template.get("name") or "") + steps = {str(node.get("agent_type") or "") for node in (template.get("graph_definition") or {}).get("nodes") or []} + missing = [ + step + for step, present in (("review", bool(steps & set(REVIEW_AGENT_TYPES))), ("QA", QA_AGENT_TYPE in steps)) + if not present + ] + if not missing: + return None + return f"{name!r} has no {' or '.join(missing)} step, and a job pinned to the full pipeline keeps review and QA" + + # --- what an agent's row says ------------------------------------------------------------------------------ diff --git a/tests/unit/agents/test_triage_may_add_skills.py b/tests/unit/agents/test_triage_may_add_skills.py index bff855ed..9e8d8e18 100644 --- a/tests/unit/agents/test_triage_may_add_skills.py +++ b/tests/unit/agents/test_triage_may_add_skills.py @@ -128,8 +128,13 @@ def _prompt(skill_choices: list[str] | None) -> str: if skill_choices is not None: params = inspect.signature(build_triage_system_prompt).parameters assert "skill_choices" in params, "build_triage_system_prompt takes no skill_choices: it cannot follow the row" + from embry0.settings.sections.slop_meter import SLOP_METER + from embry0.settings.sections.slop_policy import SLOP_POLICY + kwargs: dict[str, Any] = {"default_model": "m", "tier_models": {}} kwargs["ticket_sections"] = DONE_WHEN.defaults().ticket_sections + kwargs["slop_policy"] = SLOP_POLICY.defaults() + kwargs["level_names"] = SLOP_METER.defaults().level_names if skill_choices is not None: kwargs["skill_choices"] = skill_choices return build_triage_system_prompt(**kwargs) diff --git a/tests/unit/api/test_slop_policy_settings_live.py b/tests/unit/api/test_slop_policy_settings_live.py new file mode 100644 index 00000000..c3562ece --- /dev/null +++ b/tests/unit/api/test_slop_policy_settings_live.py @@ -0,0 +1,426 @@ +"""Slop routing is a settings section (#169), on the real app and Postgres. + +Through the real lifespan, the real settings store and the real pipeline +templates, seeded at the start: + +- the acceptance criterion: with ``slop_policy.enabled: false`` in acme's + layer, a job of acme's judged slop takes its normal route, while globex's + still runs the cleanup; undoing the change routes acme's again; +- every write path checks the section: 422 naming each refused key, a route + template that does not take a code job included; +- a template a document routes to cannot be deleted, renamed or changed to + stop taking a code job (409 naming the documents), and the check cannot + race a settings write either way. + +Docker is stubbed. +""" + +from __future__ import annotations + +import asyncio +import json +import os +from collections.abc import AsyncIterator +from pathlib import Path +from typing import Any +from unittest.mock import patch + +import pytest +from httpx import ASGITransport, AsyncClient + +from embry0.api.app import create_app, lifespan +from embry0.config import Embry0Config +from embry0.services.job_settings import STATE_KEY, read_for_job +from embry0.settings.document import DocumentInvalid +from embry0.settings.scope import Scope +from embry0.settings.sections import CATALOG +from embry0.settings.store import SettingsStore +from embry0.storage.database import DatabasePool +from embry0.storage.repositories.pipeline_templates import PipelineTemplatesRepository +from embry0.storage.repositories.settings_documents import SettingsDocumentsRepository +from embry0.storage.seeds.pipeline_templates_builtin import seed_builtin_pipeline_templates +from embry0.workflows.issue_to_pr.plan_route import plan_route_node +from tests.unit.metrics.test_lifespan_wiring import stub_container_runtime # noqa: F401 - a fixture + +pytestmark = pytest.mark.requires_postgres + +URL = "/api/v1/config/settings" +TEMPLATES = "/api/v1/pipelines/templates" +HEADERS = {"X-Requested-With": "XMLHttpRequest"} +SLOP = {"nature": "slop", "slop_score": 72, "kinds": ["symptom_patch"], "evidence": []} + + +def _config(db: DatabasePool, tmp_path: Path) -> Embry0Config: + with patch.dict(os.environ, {}, clear=True): + return Embry0Config( # type: ignore[call-arg] + _env_file=None, + database_url=db._url, # noqa: SLF001 + auth_dev_mode=True, + webhook_dev_mode=True, + api_key="", + watcher_enabled=False, + volume_reaper_enabled=False, + linear_reconcile_mode="off", + telegram_bot_token="", + xai_proxy_enabled=False, + auth_proxy_enabled=False, + context_proxy_enabled=False, + leads_enabled=False, + slop_meter_interval_seconds=0, + slop_meter_data_path=str(tmp_path / "slop-meter"), + ) + + +@pytest.fixture +async def app( + db_with_migrations: DatabasePool, + stub_container_runtime: Any, # noqa: F811 + tmp_path: Path, +) -> AsyncIterator[Any]: + """The real app through its real lifespan, with acme and globex registered.""" + app = create_app(_config(db_with_migrations, tmp_path)) + async with lifespan(app): + for owner in ("acme", "globex"): + await app.state.organizations_repo.register(owner, "github") + for repo in ("acme/widgets", "globex/api"): + await app.state.repos_repo.register(repo, forge="github") + yield app + + +async def _call(app: Any, method: str, url: str, **kwargs: Any) -> Any: + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test", headers=HEADERS) as client: + return await client.request(method, url, **kwargs) + + +async def _patch(app: Any, keys: dict[str, Any], **params: str) -> Any: + return await _call( + app, + "PATCH", + f"{URL}/slop_policy", + content=json.dumps(keys), + headers={"Content-Type": "application/json"}, + params=params, + ) + + +async def _route(app: Any, repo: str) -> tuple[str | None, str]: + """The route plan_route gives a job of ``repo`` judged slop, with the settings it reads when it starts.""" + state = { + "job_id": f"job-{repo}", + "repo": repo, + "triage_decision": {"job_kind": "code", "work_nature": SLOP}, + STATE_KEY: await read_for_job(app.state.settings, repo), + } + configurable = {"pipeline_templates_repo": app.state.templates_repo, "db": app.state.db} + with patch("embry0.workflows.issue_to_pr.plan_route.get_stream_writer", return_value=lambda _e: None): + out = await plan_route_node(state, {"configurable": configurable}) + return out["route_template_name"], out["coding_agent"] + + +async def _template_id(app: Any, name: str) -> str: + rows = (await _call(app, "GET", TEMPLATES)).json() + found = [row["id"] for row in rows if row["name"] == name] + assert found, f"no template named {name!r}" + return str(found[0]) + + +# ---- the acceptance criterion ---- + + +async def test_with_the_policy_off_a_job_judged_slop_takes_its_normal_route(app: Any) -> None: + assert await _route(app, "acme/widgets") == ("slop-cleanup", "slopmaster"), "routed by default, as before" + + resp = await _patch(app, {"enabled": False}, owner="acme") + assert resp.status_code == 200, f"acme's layer cannot turn slop routing off: {resp.text}" + assert await _route(app, "acme/widgets") == ("full-magnum-opus", "developer"), ( + "with slop_policy.enabled: false, acme's job judged slop takes its normal route" + ) + assert await _route(app, "globex/api") == ("slop-cleanup", "slopmaster"), "acme's layer never routes globex's" + + revisions = (await _call(app, "GET", f"{URL}/revisions", params={"owner": "acme"})).json()["revisions"] + undo = await _call(app, "POST", f"{URL}/revisions/{revisions[0]['id']}/undo") + assert undo.status_code == 200, f"the change has history with undo: {undo.text}" + assert await _route(app, "acme/widgets") == ("slop-cleanup", "slopmaster"), "undone: routed again" + + +async def test_triage_leans_by_the_level_names_its_repository_reads(app: Any) -> None: + from embry0.services.slop_meter import context + + names = ["Clean", "Fine", "Mixed", "Sloppy", "Slop"] + resp = await _call( + app, + "PATCH", + f"{URL}/slop_meter", + content=json.dumps({"level_names": names}), + headers={"Content-Type": "application/json"}, + params={"owner": "acme"}, + ) + assert resp.status_code == 200, resp.text + reader = getattr(context, "level_names", None) + assert reader is not None, "triage is told no level names but the defaults" + config = {"configurable": {"db": app.state.db}} + assert list(await reader(config, "acme/widgets")) == names, "acme's own names" + assert (await reader(config, "globex/api"))[3] == "Sloppy Joe", "globex reads the defaults" + + +# ---- the write paths ---- + + +async def test_a_write_names_each_refused_key(app: Any) -> None: + text = ( + "slop_policy:\n kinds: []\n route_template: research-default\n ticket_heading: '## Slop'\n" + " dungeon_min_level: 9\n" + ) + resp = await _call(app, "PUT", URL, content=text, headers={"Content-Type": "application/yaml"}) + assert resp.status_code == 422, resp.text + named = {".".join(map(str, error["loc"][-2:])): error["msg"] for error in resp.json()["detail"]} + assert set(named) == { + "slop_policy.kinds", + "slop_policy.route_template", + "slop_policy.ticket_heading", + "slop_policy.dungeon_min_level", + }, f"the 422 names each refused key: {named}" + assert "records its jobs as research jobs" in named["slop_policy.route_template"] + + unknown = await _patch(app, {"route_template": "nowhere"}, owner="acme") + assert unknown.status_code == 422 and "no pipeline template named 'nowhere'" in unknown.text, unknown.text + words = await _patch(app, {"classifier_guide": "Our own words."}, repo="acme/widgets") + assert words.status_code == 422 and "not_overridable" in words.text, words.text + fine = await _patch(app, {"route_template": "quick-fix", "min_score_to_route": 50}, repo="acme/widgets") + assert fine.status_code == 200, fine.text + + +async def test_the_policy_tab_shows_the_section(app: Any) -> None: + effective = (await _call(app, "GET", f"{URL}/effective")).json() + text = json.dumps(effective) + assert "slop_policy" in text and "route_template" in text and "classifier_guide" in text, ( + "the Policy tab lists the sections the API answers" + ) + schema = (await _call(app, "GET", f"{URL}/schema")).json() + assert schema["x-embry0-sections"]["slop_policy"]["repo_keys"] == [ + "enabled", + "min_score_to_route", + "route_template", + ] + + +# ---- a template a policy routes to ---- + + +async def test_a_template_a_policy_routes_to_is_not_deleted_renamed_or_unfitted(app: Any) -> None: + cleanup = await _template_id(app, "slop-cleanup") + gone = await _call(app, "DELETE", f"{TEMPLATES}/{cleanup}") + assert gone.status_code == 409, f"the template the default policy routes to was deleted: {gone.text}" + assert "the deployment's document (its default)" in gone.json()["detail"] + renamed = await _call(app, "PUT", f"{TEMPLATES}/{cleanup}", json={"name": "tidy"}) + assert renamed.status_code == 409 and "rename it" in renamed.json()["detail"], ( + f"a template a document routes to was renamed: {renamed.text}" + ) + described = await _call(app, "PUT", f"{TEMPLATES}/{cleanup}", json={"description": "Our cleanup."}) + assert described.status_code == 200, "a change that keeps it a code route is taken" + + copy = await _call(app, "POST", f"{TEMPLATES}/{cleanup}/duplicate", json={"name": "tidy-up"}) + assert copy.status_code == 201, copy.text + tidy = str(copy.json()["id"]) + assert (await _patch(app, {"route_template": "tidy-up"}, owner="acme")).status_code == 200 + held = await _call(app, "DELETE", f"{TEMPLATES}/{tidy}") + assert held.status_code == 409 and "organization acme" in held.json()["detail"], held.text + research = await _call(app, "PUT", f"{TEMPLATES}/{tidy}", json={"job_kind": "research"}) + assert research.status_code == 409 and "no longer takes a code job" in research.json()["detail"], research.text + assert await _route(app, "acme/widgets") == ("tidy-up", "slopmaster") + + assert (await _patch(app, {"route_template": "tidy-up"})).status_code == 200 + released = await _call(app, "DELETE", f"{TEMPLATES}/{cleanup}") + assert released.status_code == 200, f"nothing routes to slop-cleanup now: {released.text}" + + +async def _tidy_up(app: Any, **fields: Any) -> str: + """A copy of slop-cleanup named tidy-up, with ``fields`` changed; its id.""" + copy = await _call( + app, "POST", f"{TEMPLATES}/{await _template_id(app, 'slop-cleanup')}/duplicate", json={"name": "tidy-up"} + ) + assert copy.status_code == 201, copy.text + tidy = str(copy.json()["id"]) + if fields: + changed = await _call(app, "PUT", f"{TEMPLATES}/{tidy}", json=fields) + assert changed.status_code == 200, changed.text + return tidy + + +async def test_a_template_whose_row_records_another_kind_is_refused(app: Any) -> None: + """The write reads each template's row as it is, its own job_kind included.""" + await _tidy_up(app, job_kind="research") + refused = await _patch(app, {"route_template": "tidy-up"}, owner="acme") + assert refused.status_code == 422 and "records its jobs as research jobs" in refused.text, ( + f"a template whose row records research jobs was taken as a slop route: {refused.text}" + ) + + +async def test_a_dry_run_checks_the_template_as_a_write_does(app: Any) -> None: + resp = await _call( + app, + "PATCH", + f"{URL}/slop_policy", + content=json.dumps({"route_template": "nowhere"}), + headers={"Content-Type": "application/json"}, + params={"owner": "acme", "dry_run": "true"}, + ) + assert resp.status_code == 422 and "no pipeline template named 'nowhere'" in resp.text, ( + f"a dry run passed a template the write would refuse: {resp.status_code} {resp.text}" + ) + + +async def test_taking_route_template_out_never_leaves_a_template_that_is_gone(app: Any) -> None: + """The deployment routes to tidy-up and slop-cleanup is deleted: a write that falls back to the default, + by leaving the key out or the whole section, is refused, and tidy-up stays held.""" + tidy = await _tidy_up(app) + assert (await _patch(app, {"route_template": "tidy-up"})).status_code == 200 + assert (await _call(app, "DELETE", f"{TEMPLATES}/{await _template_id(app, 'slop-cleanup')}")).status_code == 200 + for text in ("slop_policy:\n enabled: true\n", "ui:\n game: true\n"): + resp = await _call(app, "PUT", URL, content=text, headers={"Content-Type": "application/yaml"}) + assert resp.status_code == 422, ( + f"{text!r} left the deployment routing to the default slop-cleanup, which is gone: {resp.text}" + ) + locs = [".".join(map(str, error["loc"][-2:])) for error in resp.json()["detail"]] + assert locs == ["slop_policy.route_template"], locs + assert await _route(app, "acme/widgets") == ("tidy-up", "slopmaster") + held = await _call(app, "DELETE", f"{TEMPLATES}/{tidy}") + assert held.status_code == 409, "tidy-up is still what the deployment routes to" + + +async def test_an_undo_that_would_route_to_a_template_that_is_gone_is_refused(app: Any) -> None: + tidy = await _tidy_up(app) + assert (await _patch(app, {"route_template": "tidy-up"}, owner="acme")).status_code == 200 + assert (await _patch(app, {"route_template": "slop-cleanup"}, owner="acme")).status_code == 200 + assert (await _call(app, "DELETE", f"{TEMPLATES}/{tidy}")).status_code == 200, "nothing routes to tidy-up now" + revisions = (await _call(app, "GET", f"{URL}/revisions", params={"owner": "acme"})).json()["revisions"] + undo = await _call(app, "POST", f"{URL}/revisions/{revisions[0]['id']}/undo") + assert undo.status_code == 422 and "no pipeline template named 'tidy-up'" in undo.text, ( + f"an undo put back a route to a deleted template: {undo.status_code} {undo.text}" + ) + assert await _route(app, "acme/widgets") == ("slop-cleanup", "slopmaster") + + +async def test_a_repository_layer_holds_the_template_it_routes_to(app: Any) -> None: + tidy = await _tidy_up(app) + assert (await _patch(app, {"route_template": "tidy-up"}, repo="acme/widgets")).status_code == 200 + held = await _call(app, "DELETE", f"{TEMPLATES}/{tidy}") + assert held.status_code == 409 and "repository acme/widgets" in held.json()["detail"], ( + f"a template a repository's layer routes to was deleted: {held.text}" + ) + assert (await _patch(app, {"route_template": None}, repo="acme/widgets")).status_code == 200 + assert (await _call(app, "DELETE", f"{TEMPLATES}/{tidy}")).status_code == 200, "released by editing that layer" + + +# ---- no race between a settings write and a template change ---- + + +def _guarding() -> Any: + """``embry0.services.slop_routing``: the guard of a template a settings document routes to.""" + try: + from embry0.services import slop_routing + except ImportError: + pytest.fail("nothing guards a template a settings document routes slop jobs to") + return slop_routing + + +async def _seeded(db: DatabasePool) -> tuple[PipelineTemplatesRepository, str]: + templates = PipelineTemplatesRepository(db) + await seed_builtin_pipeline_templates(templates) + rows = {row["name"]: row["id"] for row in await templates.list_all()} + copy = await templates.duplicate(str(rows["slop-cleanup"]), new_name="tidy-up") + return templates, str(copy["id"]) + + +async def _settle(go: asyncio.Event, *tasks: asyncio.Task[Any]) -> None: + """Let a held transaction go and wait for every task, whatever the test asserted: a task left waiting holds + its transaction open, and the database fixture's cleanup would wait for it for good.""" + go.set() + await asyncio.wait(tasks, timeout=30) + for task in tasks: + if not task.done(): + task.cancel() + + +async def test_a_delete_waits_for_a_write_that_read_its_template_and_then_sees_it( + db_with_migrations: DatabasePool, +) -> None: + db = db_with_migrations + routing = _guarding() + templates, tidy = await _seeded(db) + locked, go = asyncio.Event(), asyncio.Event() + from embry0.settings.write_context import deployment_reads + + async def slow_reads(conn: Any, sections: frozenset[str]) -> Any: + context = await deployment_reads(conn, sections) + locked.set() + await go.wait() + return context + + store = SettingsStore(db, write_reads=slow_reads) + write = asyncio.create_task( + store.set_keys("slop_policy", {"route_template": "tidy-up"}, actor="t", scope=Scope.organization("acme")) + ) + guard = routing.slop_route_guard(SettingsDocumentsRepository(db), CATALOG) + delete: asyncio.Task[Any] | None = None + try: + await asyncio.wait_for(locked.wait(), 10) + delete = asyncio.create_task(templates.delete(tidy, guard=guard)) + await asyncio.sleep(0.3) + blocked = not delete.done() + finally: + await _settle(go, write, *([delete] if delete is not None else [])) + assert blocked, "the delete waits for the settings write that read its template" + write.result() + with pytest.raises(routing.TemplateInUse, match="organization acme"): + delete.result() + assert await templates.get(tidy) is not None + + +async def test_a_write_waits_for_a_delete_of_its_template_and_then_refuses_it(db_with_migrations: DatabasePool) -> None: + db = db_with_migrations + routing = _guarding() + templates, tidy = await _seeded(db) + locked, go = asyncio.Event(), asyncio.Event() + guard = routing.slop_route_guard(SettingsDocumentsRepository(db), CATALOG) + + async def slow_guard(conn: Any, before: Any, after: Any) -> None: + await guard(conn, before, after) + locked.set() + await go.wait() + + delete = asyncio.create_task(templates.delete(tidy, guard=slow_guard)) + store = SettingsStore(db) + write: asyncio.Task[Any] | None = None + try: + await asyncio.wait_for(locked.wait(), 10) + write = asyncio.create_task( + store.set_keys("slop_policy", {"route_template": "tidy-up"}, actor="t", scope=Scope.organization("acme")) + ) + await asyncio.sleep(0.3) + blocked = not write.done() + finally: + await _settle(go, delete, *([write] if write is not None else [])) + assert blocked, "the settings write waits for the delete of the template it names" + delete.result() + with pytest.raises(DocumentInvalid) as refused: + write.result() + assert "no pipeline template named 'tidy-up'" in str([p.msg for p in refused.value.problems]) + + +async def test_only_a_write_that_may_change_slop_policy_waits_on_a_template(db_with_migrations: DatabasePool) -> None: + """A template row held by a change waits a slop_policy write, and no write of another section.""" + db = db_with_migrations + templates, tidy = await _seeded(db) + store = SettingsStore(db) + async with db.transaction() as conn: + await conn.execute("SELECT 1 FROM pipeline_templates WHERE id = $1 FOR UPDATE", tidy) + other = asyncio.create_task(store.set_keys("done_when", {"retries": 2}, actor="t")) + dry = asyncio.create_task(store.preview_keys("done_when", {"retries": 3})) + slop = asyncio.create_task(store.set_keys("slop_policy", {"min_score_to_route": 10}, actor="t")) + done, _pending = await asyncio.wait({other, dry}, timeout=5) + waiting = not slop.done() + await asyncio.wait({other, dry, slop}, timeout=30) + assert done == {other, dry}, "a write of another section, and its dry run, take no lock on the templates" + assert waiting, "a slop_policy write reads the templates FOR SHARE, so it waits for the held row" + slop.result() diff --git a/tests/unit/api/test_watcher_wiring.py b/tests/unit/api/test_watcher_wiring.py index f16fcbc2..2df08690 100644 --- a/tests/unit/api/test_watcher_wiring.py +++ b/tests/unit/api/test_watcher_wiring.py @@ -41,6 +41,8 @@ "auto_acceptor", # #172 — without this a mask stored after startup never reaches a tick. "signature_masks", + # #169 — without this a stack's slop_policy never reaches its analyses or tickets. + "slop_policy", ) diff --git a/tests/unit/api/v1/test_behaviour_writes_api.py b/tests/unit/api/v1/test_behaviour_writes_api.py index df23d922..10bec30f 100644 --- a/tests/unit/api/v1/test_behaviour_writes_api.py +++ b/tests/unit/api/v1/test_behaviour_writes_api.py @@ -212,9 +212,8 @@ async def test_a_templates_update_writes_what_it_sends_and_clears_what_it_nulls( ): resp = await _send(app, "PUT", "/api/v1/pipelines/templates/t-1", body) assert resp.status_code == 200, resp.text - assert app.state.templates_repo.update.call_args.kwargs == want, ( - f"{body}: {app.state.templates_repo.update.call_args}" - ) + sent = {k: v for k, v in app.state.templates_repo.update.call_args.kwargs.items() if k != "guard"} + assert sent == want, f"{body}: {app.state.templates_repo.update.call_args}" @pytest.mark.parametrize( diff --git a/tests/unit/api/v1/test_pipeline_templates_api.py b/tests/unit/api/v1/test_pipeline_templates_api.py index c73c9101..45f69aad 100644 --- a/tests/unit/api/v1/test_pipeline_templates_api.py +++ b/tests/unit/api/v1/test_pipeline_templates_api.py @@ -401,7 +401,7 @@ async def test_an_explicit_null_clears_the_default_kind(app): async with AsyncClient(transport=transport, base_url="http://test") as client: resp = await client.put("/api/v1/pipelines/templates/tmpl-1", json={"default_for_kind": None}, headers=_XHR) assert resp.status_code == 200, resp.text - app.state.templates_repo.update.assert_awaited_once_with("tmpl-1", default_for_kind=None) + app.state.templates_repo.update.assert_awaited_once_with("tmpl-1", guard=None, default_for_kind=None) @pytest.mark.asyncio @@ -410,7 +410,7 @@ async def test_an_absent_default_kind_is_kept(app): async with AsyncClient(transport=transport, base_url="http://test") as client: resp = await client.put("/api/v1/pipelines/templates/tmpl-1", json={"name": "Renamed"}, headers=_XHR) assert resp.status_code == 200, resp.text - app.state.templates_repo.update.assert_awaited_once_with("tmpl-1", name="Renamed") + app.state.templates_repo.update.assert_awaited_once_with("tmpl-1", guard=None, name="Renamed") @pytest.mark.asyncio diff --git a/tests/unit/api/v1/test_world_slop_policy.py b/tests/unit/api/v1/test_world_slop_policy.py new file mode 100644 index 00000000..10945f28 --- /dev/null +++ b/tests/unit/api/v1/test_world_slop_policy.py @@ -0,0 +1,67 @@ +"""The world draws an area's dungeon from its repository's ``slop_policy.dungeon_min_level`` (#169).""" + +from __future__ import annotations + +from typing import Any + +import pytest +from httpx import AsyncClient + +from embry0.api.v1 import world as world_api +from embry0.settings.sections import CATALOG +from embry0.settings.store import build_snapshot +from tests.unit.api.v1.test_world_api import NOW, _client, app # noqa: F401 - app is a fixture + +#: An area of acme's at Lasagna (level 3) with no open slop finding and no cleanup. +SEARCH = { + "repo": "acme/web", + "area": "Search", + "score": 0.52, + "level": 3, + "name": "Lasagna", + "provisional": False, + "computed_at": NOW, +} + + +class _Meters: + async def scores(self, repo: str | None = None) -> list[dict[str, Any]]: + return [SEARCH] + + +class _Settings: + def __init__(self, level: Any) -> None: + self.rows = [ + {"scope": "deployment", "data": {}, "generation": 1, "updated_at": None}, + {"scope": "organization:acme", "data": {"slop_policy": {"dungeon_min_level": level}}, "generation": 1}, + ] + + async def snapshot(self) -> Any: + return build_snapshot(CATALOG, self.rows, token=(), log=False) + + +async def _areas(app: Any) -> set[tuple[str, str | None]]: # noqa: F811 - the fixture's value + client: AsyncClient + async with await _client(app) as client: + world = (await client.get("/api/v1/world")).json() + return {(d["repo"], d["area"]) for d in world["dungeons"]} + + +@pytest.fixture +def no_leads(monkeypatch: pytest.MonkeyPatch) -> None: + async def none(request: Any) -> list[dict[str, Any]]: + return [] + + monkeypatch.setattr(world_api, "_leads", none) + + +@pytest.mark.usefixtures("no_leads") +async def test_an_area_has_a_dungeon_from_its_organization_s_level(app: Any) -> None: # noqa: F811 + app.state.slop_meter_repo = _Meters() + assert ("acme/web", "Search") in await _areas(app), "level 2 by default: Lasagna has a dungeon" + app.state.settings = _Settings(4) + assert ("acme/web", "Search") not in await _areas(app), "acme draws from Sloppy Joe" + app.state.settings = _Settings(None) + assert ("acme/web", "Search") not in await _areas(app), "acme's meter alone draws none" + app.state.settings = _Settings(3) + assert ("acme/web", "Search") in await _areas(app) diff --git a/tests/unit/orchestration/nodes/test_triage_model_guidance.py b/tests/unit/orchestration/nodes/test_triage_model_guidance.py index 2593f93a..db8e9b20 100644 --- a/tests/unit/orchestration/nodes/test_triage_model_guidance.py +++ b/tests/unit/orchestration/nodes/test_triage_model_guidance.py @@ -36,11 +36,15 @@ def _prompt(default_model: str = "", tiers: dict[str, str] | None = None) -> str: from embry0.orchestration.nodes.triage import build_triage_system_prompt from embry0.settings.sections.done_when import DONE_WHEN + from embry0.settings.sections.slop_meter import SLOP_METER + from embry0.settings.sections.slop_policy import SLOP_POLICY return build_triage_system_prompt( default_model=default_model, tier_models=tiers if tiers is not None else _TIERS, ticket_sections=DONE_WHEN.defaults().ticket_sections, + slop_policy=SLOP_POLICY.defaults(), + level_names=SLOP_METER.defaults().level_names, ) diff --git a/tests/unit/orchestration/nodes/test_triage_node.py b/tests/unit/orchestration/nodes/test_triage_node.py index 79cb591b..0ea781f0 100644 --- a/tests/unit/orchestration/nodes/test_triage_node.py +++ b/tests/unit/orchestration/nodes/test_triage_node.py @@ -446,8 +446,14 @@ def test_parse_triage_response_keeps_its_done_when_list(): raw = json.dumps({"action": "proceed", "confidence": 0.8, "reasoning": "r", "done_when": listed}) assert parse_triage_response(raw)["done_when"] == listed from embry0.settings.sections.done_when import DONE_WHEN + from embry0.settings.sections.slop_meter import SLOP_METER + from embry0.settings.sections.slop_policy import SLOP_POLICY prompt = build_triage_system_prompt( - default_model="m", tier_models={}, ticket_sections=DONE_WHEN.defaults().ticket_sections + default_model="m", + tier_models={}, + ticket_sections=DONE_WHEN.defaults().ticket_sections, + slop_policy=SLOP_POLICY.defaults(), + level_names=SLOP_METER.defaults().level_names, ) assert "## Done when" in prompt and "<<" not in prompt diff --git a/tests/unit/orchestration/nodes/test_triage_slop_policy.py b/tests/unit/orchestration/nodes/test_triage_slop_policy.py new file mode 100644 index 00000000..f3441892 --- /dev/null +++ b/tests/unit/orchestration/nodes/test_triage_slop_policy.py @@ -0,0 +1,169 @@ +"""Triage's "Bug or slop" section says what the job's ``slop_policy`` says (#169). + +With the defaults the system prompt is the one ``origin/main`` built, to the +byte (its digest was taken on ``c80ba42e`` with the same arguments). With an +organization's policy it carries that organization's guide and heading, says +what a verdict does to the route, and leans by the meter's own level names; +an operator's text is never read for a slot's marker. +""" + +from __future__ import annotations + +import hashlib +from typing import Any +from unittest.mock import AsyncMock, patch + +import pytest + +from embry0.orchestration.nodes.triage import build_triage_system_prompt +from embry0.services.job_settings import STATE_KEY +from embry0.settings.sections import CATALOG +from embry0.settings.sections.done_when import DONE_WHEN +from embry0.settings.sections.slop_meter import SLOP_METER + +TIERS = {"model_heavy": "tier-heavy", "model_medium": "tier-medium", "model_light": "tier-light"} +SKILLS = ("superpowers:brainstorming", "superpowers:writing-plans") +#: ``sha256`` of the prompt ``build_triage_system_prompt`` built on ``c80ba42e`` (origin/main before the section) +#: with these tiers, the default ticket headings and these skills; and with one default model instead. +MAIN_PROMPT = "282bde4bc3087d13fc0ee3bb72e37e6710d6ce1705e2185a8540c252ea1cb3d2" +MAIN_PROMPT_ONE_MODEL = "70db086d7eeef80c48f73873c2c593821fdc5e808ab1e5973634699563abd965" +#: The paragraph after the guide on ``c80ba42e``. +MAIN_PARAGRAPH = """\ +Fill `work_nature` for every code job, with at most 8 evidence entries naming +files you read. When the task carries a watcher "Slop, not a bug" section, +weigh it, then make your own call. A slop verdict routes the job to the +Slopmaster's cleanup (keep behavior, remove or merge code) whatever template +you name; leave pipeline_template empty for it. The task context may give the +area's Slop-O-Meter reading (how much of its code landed beyond human +attention); when your verdict is borderline and it reads Sloppy Joe or Just +The Slop, lean toward slop.""" +DEFAULT_NAMES = ("Buttered Noodles", "Spaghetti Bolognese", "Lasagna", "Sloppy Joe", "Just The Slop") + + +def _policy(values: dict[str, Any] | None = None) -> Any: + section = CATALOG.get("slop_policy") + if section is None: + pytest.fail("the settings document has no slop_policy section: triage's words about slop are code") + return section.validate(values or {}) + + +def _prompt(policy: dict[str, Any] | None = None, *, names: tuple[str, ...] = DEFAULT_NAMES, model: str = "") -> str: + try: + return build_triage_system_prompt( + default_model=model, + tier_models=TIERS, + ticket_sections=DONE_WHEN.defaults().ticket_sections, + slop_policy=_policy(policy), + level_names=names, + skill_choices=SKILLS, + ) + except TypeError as exc: + pytest.fail(f"triage's prompt is built from no slop_policy: {exc}") + + +def _digest(text: str) -> str: + return hashlib.sha256(text.encode()).hexdigest() + + +def _flat(text: str) -> str: + """The words of ``text``, wrapped lines joined: the paragraph is wrapped wherever its words fall.""" + return " ".join(text.split()) + + +def test_the_default_prompt_is_the_one_triage_was_given_before_the_policy() -> None: + assert SLOP_METER.defaults().level_names == DEFAULT_NAMES + prompt = _prompt() + assert MAIN_PARAGRAPH in prompt + assert _digest(prompt) == MAIN_PROMPT, "the default triage prompt differs from the one before #169" + assert _digest(_prompt(model="one-model")) == MAIN_PROMPT_ONE_MODEL + + +def test_the_prompt_says_what_the_organization_s_policy_says() -> None: + prompt = _prompt( + { + "classifier_guide": "Our slop is code that no test reaches.\nKinds: untested.", + "kinds": ["untested"], + "ticket_heading": "Shape, not behaviour", + "min_score_to_route": 60, + } + ) + assert "Our slop is code that no test reaches.\nKinds: untested." in prompt + assert "Tell slop apart from bugs" not in prompt, "the organization's guide replaces the default" + assert 'watcher "Shape, not behaviour" section' in _flat(prompt) + assert "A slop verdict with a slop_score of 60 or more routes the job" in _flat(prompt) + assert "Below that score, pick its template as for a bug." in _flat(prompt) + + +def test_triage_is_told_the_route_its_policy_names() -> None: + """Only the built-in cleanup, whose graph is seeded at every start, is said to be the Slopmaster's.""" + prompt = _flat(_prompt({"route_template": "tidy-up"})) + assert "routes the job to the cleanup pipeline `tidy-up` (keep behavior, remove or merge code)" in prompt, ( + "triage is told every slop route is the Slopmaster's cleanup" + ) + assert "Slopmaster's cleanup" not in prompt, "a route of the operator's own may be coded by the developer" + + +def test_with_routing_off_triage_is_not_told_the_verdict_routes() -> None: + prompt = _flat(_prompt({"enabled": False})) + assert "routes the job to the" not in prompt + assert "does not change the job's route here" in prompt + + +def test_triage_leans_from_the_policy_s_level_by_the_meter_s_names() -> None: + names = ("Clean", "Fine", "Mixed", "Sloppy", "Slop") + assert "it reads Mixed, Sloppy or Slop, lean toward slop" in _flat( + _prompt({"triage_lean_min_level": 3}, names=names) + ) + assert "it reads Slop, lean toward slop" in _flat(_prompt({"triage_lean_min_level": 5}, names=names)) + never = _flat(_prompt({"triage_lean_min_level": None})) + assert "lean toward slop" not in never + assert "landed beyond human attention)." in never, "the reading is still explained" + + +def test_an_operator_s_text_is_never_read_for_a_slot() -> None: + prompt = _prompt({"classifier_guide": "Keep <> and <> as written."}) + assert "Keep <> and <> as written." in prompt + assert prompt.count("Model selection (CRITICAL)") == 1 + + +@pytest.mark.asyncio +async def test_triage_is_built_from_the_policy_its_job_read() -> None: + """The node passes the job's own slop_policy (job_settings) and the meter's names for its repository.""" + from embry0.workflows.issue_to_pr.nodes import triage_node + from tests.unit.workflows.test_policy_efficiency import TRIAGE_STATE, _sessions_repo, _triage_result + + captured: dict[str, Any] = {} + + async def _run(*args: Any, **kwargs: Any) -> Any: + captured["agent_definition"] = kwargs.get("agent_definition") + return _triage_result() + + policy = _policy({"classifier_guide": "Acme calls dead flags slop.", "ticket_heading": "Shape"}) + state = {**TRIAGE_STATE, STATE_KEY: {"slop_policy": policy.model_dump(mode="json")}} + asked: list[str] = [] + + async def acme_names(config: Any, repo: str) -> tuple[str, ...]: + asked.append(repo) + return ("Clean", "Fine", "Mixed", "Sloppy", "Slop") + + with ( + patch("embry0.workflows.issue_to_pr.nodes.run_agent_node", new=AsyncMock(side_effect=_run)), + patch("embry0.workflows.issue_to_pr.nodes.get_stream_writer", return_value=lambda _: None), + patch("embry0.services.slop_meter.context.level_names", new=acme_names, create=True), + ): + await triage_node( + state, + { + "configurable": { + "agent_runner": object(), + "credentials": {}, + "agent_sessions_repo": _sessions_repo(None), + } + }, + ) + system = captured["agent_definition"]["system_prompt"] + assert "Acme calls dead flags slop." in system, "triage is told the guide its job read" + assert 'watcher "Shape" section' in _flat(system) + assert "Tell slop apart from bugs" not in system + assert asked == [TRIAGE_STATE["repo"]], "the meter's level names are read for the job's repository" + assert "it reads Sloppy or Slop, lean toward slop" in _flat(system), "triage leans by the meter's own names" diff --git a/tests/unit/services/test_job_settings.py b/tests/unit/services/test_job_settings.py index b58c44f8..78af36a4 100644 --- a/tests/unit/services/test_job_settings.py +++ b/tests/unit/services/test_job_settings.py @@ -24,6 +24,7 @@ from embry0.services import done_when as dw from embry0.services.issue_executor import IssueExecutor from embry0.settings.sections import CATALOG +from embry0.settings.sections.slop_policy import SLOP_POLICY from embry0.settings.store import build_snapshot WIDGETS = "acme/widgets" @@ -287,6 +288,8 @@ async def test_without_a_store_or_with_one_that_fails_a_job_runs_on_the_defaults }, "done_when": {"retries": 1, "ticket_sections": ["Acceptance criteria"]}, "salvage": {"commit_subject": WIP}, + # #169: what a slop verdict does to the job (tests/unit/settings/test_slop_policy_section.py pins them). + "slop_policy": SLOP_POLICY.defaults().model_dump(mode="json"), } assert await js.read_for_job(None, WIDGETS) == defaults broken = MagicMock() diff --git a/tests/unit/services/test_slop.py b/tests/unit/services/test_slop.py index 19d6a768..50c84f63 100644 --- a/tests/unit/services/test_slop.py +++ b/tests/unit/services/test_slop.py @@ -13,6 +13,13 @@ slop_brief, ticket_section, ) +from embry0.settings.sections.slop_policy import DEFAULT_DUNGEON_MIN_LEVEL, DEFAULT_KINDS, DEFAULT_TICKET_HEADING + +KINDS = DEFAULT_KINDS + + +def _default_level(_repo: str) -> int: + return DEFAULT_DUNGEON_MIN_LEVEL def test_a_slop_verdict_keeps_its_known_kinds_and_clamped_score() -> None: @@ -22,7 +29,8 @@ def test_a_slop_verdict_keeps_its_known_kinds_and_clamped_score() -> None: "slop_score": 140, "kinds": ["symptom-patch", "duplication", "vibes", "duplication"], "evidence": [{"path": "svc/outbox.py", "why": "three retry wrappers around one insert"}, "junk"], - } + }, + kinds=KINDS, ) assert verdict == { "nature": "slop", @@ -33,14 +41,14 @@ def test_a_slop_verdict_keeps_its_known_kinds_and_clamped_score() -> None: def test_no_verdict_is_better_than_a_wrong_one() -> None: - assert parse_work_nature({"nature": "feature"}) is None - assert parse_work_nature("slop") is None - assert parse_work_nature(None) is None + assert parse_work_nature({"nature": "feature"}, kinds=KINDS) is None + assert parse_work_nature("slop", kinds=KINDS) is None + assert parse_work_nature(None, kinds=KINDS) is None def test_a_missing_score_reads_from_the_nature_and_evidence_is_capped() -> None: - bug = parse_work_nature({"nature": "bug"}) - slop = parse_work_nature({"nature": "slop", "evidence": [{"path": f"f{i}.py"} for i in range(20)]}) + bug = parse_work_nature({"nature": "bug"}, kinds=KINDS) + slop = parse_work_nature({"nature": "slop", "evidence": [{"path": f"f{i}.py"} for i in range(20)]}, kinds=KINDS) assert bug is not None and bug["slop_score"] == 0 assert slop is not None and slop["slop_score"] == 100 and len(slop["evidence"]) == MAX_EVIDENCE @@ -51,14 +59,16 @@ def test_the_watcher_lines_parse_from_the_head_only() -> None: "SLOP_KINDS: symptom_patch, duplication\nSLOP_EVIDENCE: svc/outbox.py | three retry wrappers\n" "BODY:\n## Evidence\nNATURE: bug\n" ) - verdict = parse_contract_lines(text) + verdict = parse_contract_lines(text, kinds=KINDS) assert verdict == { "nature": "slop", "slop_score": 72, "kinds": ["symptom_patch", "duplication"], "evidence": [{"path": "svc/outbox.py", "why": "three retry wrappers"}], } - assert parse_contract_lines("TITLE: x\nBODY:\nNATURE: slop\n") is None, "a body quoting the line is no verdict" + assert parse_contract_lines("TITLE: x\nBODY:\nNATURE: slop\n", kinds=KINDS) is None, ( + "a body quoting the line is no verdict" + ) def test_the_ticket_and_the_prompt_say_it_is_slop() -> None: @@ -68,8 +78,8 @@ def test_the_ticket_and_the_prompt_say_it_is_slop() -> None: "kinds": ["symptom_patch"], "evidence": [{"path": "a.py", "why": "x"}], } - assert ticket_section(verdict).startswith("## Slop, not a bug\n") - assert "slop score 72/100: symptom patch" in ticket_section(verdict) + assert ticket_section(verdict, heading=DEFAULT_TICKET_HEADING).startswith("## Slop, not a bug\n") + assert "slop score 72/100: symptom patch" in ticket_section(verdict, heading=DEFAULT_TICKET_HEADING) brief = slop_brief(verdict) assert "Do not add guards" in brief and "- a.py: x" in brief assert slop_brief({"nature": "bug", "slop_score": 0, "kinds": [], "evidence": []}) == "" @@ -86,7 +96,7 @@ def test_a_dungeon_rises_with_slop_and_falls_as_it_is_cleaned() -> None: {"repo": "acme/web", "area": None, "nature": "bug", "slop_score": 0}, {"repo": "acme/api", "area": None, "nature": "slop", "slop_score": 30}, ] - found = dungeons(leads, {("acme/web", "Billing"): 2, ("acme/old", None): 1}) + found = dungeons(leads, {("acme/web", "Billing"): 2, ("acme/old", None): 1}, (), min_level=_default_level) assert found == [ {"repo": "acme/web", "area": "Billing", "score": 80, "open": 2, "cleaned": 2, "meter": None}, {"repo": "acme/api", "area": None, "score": 30, "open": 1, "cleaned": 0, "meter": None}, @@ -127,7 +137,7 @@ def test_a_dungeon_carries_its_meter_and_a_sloppy_area_has_one_without_findings( }, ] leads = [{"repo": "acme/web", "area": "Billing", "nature": "slop", "slop_score": 60}] - found = {(d["repo"], d["area"]): d for d in dungeons(leads, {}, meters)} + found = {(d["repo"], d["area"]): d for d in dungeons(leads, {}, meters, min_level=_default_level)} assert set(found) == {("acme/web", "Billing"), ("acme/api", None)}, "a clean repository has no dungeon of its own" assert found[("acme/web", "Billing")]["meter"] == { "score": 0.73, diff --git a/tests/unit/services/test_slop_framing.py b/tests/unit/services/test_slop_framing.py index fe9870f1..3a3f03da 100644 --- a/tests/unit/services/test_slop_framing.py +++ b/tests/unit/services/test_slop_framing.py @@ -75,11 +75,15 @@ def test_a_merged_cleanup_is_priced_by_its_net_lines_removed() -> None: def test_triage_is_asked_for_the_verdict_and_a_bad_one_never_fails_it() -> None: from embry0.settings.sections.done_when import DONE_WHEN + from embry0.settings.sections.slop_meter import SLOP_METER + from embry0.settings.sections.slop_policy import SLOP_POLICY prompt = build_triage_system_prompt( default_model="m", tier_models={"model_heavy": "h", "model_medium": "m", "model_light": "l"}, ticket_sections=DONE_WHEN.defaults().ticket_sections, + slop_policy=SLOP_POLICY.defaults(), + level_names=SLOP_METER.defaults().level_names, ) assert "## Bug or slop" in prompt and "symptom_patch" in prompt and "<>" not in prompt decision = parse_triage_response(json.dumps({"action": "proceed", "confidence": 0.8, "work_nature": SLOP})) diff --git a/tests/unit/services/test_slop_policy_readers.py b/tests/unit/services/test_slop_policy_readers.py new file mode 100644 index 00000000..785fe67f --- /dev/null +++ b/tests/unit/services/test_slop_policy_readers.py @@ -0,0 +1,135 @@ +"""Each reader of ``slop_policy`` reads its own repository's (#169): a job, the reviewer, the ticket, the world. + +The outputs with the defaults are pinned as ``origin/main`` (``c80ba42e``) +wrote them, and an organization's layer reaches its own repositories only. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from typing import Any + +import pytest + +from embry0.services.job_settings import STATE_KEY, job_section, read_for_job +from embry0.services.slop import dungeons, ticket_section +from embry0.settings.sections import CATALOG +from embry0.settings.store import build_snapshot +from embry0.workflows.issue_to_pr.nodes._review import _slop_review_section + +VERDICT = { + "nature": "slop", + "slop_score": 72, + "kinds": ["symptom_patch", "duplication"], + "evidence": [{"path": "svc/outbox.py", "why": "three retry wrappers"}], +} +#: What ``ticket_section`` and ``_slop_review_section`` wrote on ``c80ba42e`` for these inputs. +MAIN_TICKET = ( + "## Slop, not a bug\nThe watcher judged this slop (slop score 72/100: symptom patch, duplication). The code " + "does what it says; the trouble is its shape. Clean it up — keep behavior, remove or merge code — " + "instead of adding another guard or retry.\n- `svc/outbox.py`: three retry wrappers\n\n" +) +MAIN_REVIEW = ( + "## This change is a slop cleanup\n\nIt was judged slop, not a bug: it must keep behavior and improve shape.\n" + "Approve only when all of these hold, and request changes otherwise, saying which:\n- behavior is unchanged: " + "the tests pass, and no public behavior, API or output\n moved (removing what nothing reaches is fine);\n- the " + "change removes or merges more than it adds, or is plainly simpler;\n- it adds no new guard, retry, fallback, " + "catch-all or flag to hide a symptom.\nThe branch adds 5 lines and removes 90 against its base.\n" +) +ROWS: list[dict[str, Any]] = [ + {"scope": "deployment", "data": {}, "generation": 1, "updated_at": None}, + { + "scope": "organization:acme", + "data": { + "slop_policy": { + "enabled": False, + "review_guide": "Acme reviews a cleanup by its tests alone.", + "classifier_guide": "Acme's own words.", + "dungeon_min_level": 4, + } + }, + "generation": 1, + }, + {"scope": "repository:acme/legacy", "data": {"slop_policy": {"enabled": True}}, "generation": 1}, +] + + +class _Store: + async def snapshot(self) -> Any: + return build_snapshot(CATALOG, ROWS, token=(), log=False) + + +def _section() -> Any: + section = CATALOG.get("slop_policy") + if section is None: + pytest.fail("the settings document has no slop_policy section: slop routing is code") + return section + + +def _ticket(heading: str) -> str: + try: + return ticket_section(VERDICT, heading=heading) + except TypeError: + pytest.fail("a slop ticket's heading is fixed in the code") + + +def test_the_default_ticket_and_review_are_what_they_were() -> None: + assert _ticket(_section().defaults().ticket_heading) == MAIN_TICKET + state = {"coding_agent": "slopmaster", "work_nature": {**VERDICT, "cleanup": {"added": 5, "removed": 90}}} + assert _slop_review_section(state) == MAIN_REVIEW, "a job that read no policy reviews as before" + assert _ticket("Shape, not behaviour").startswith("## Shape, not behaviour\n") + + +@pytest.mark.asyncio +async def test_a_job_reads_its_own_repository_s_policy_and_its_reviewer_hears_it() -> None: + section = _section() + acme = {STATE_KEY: await read_for_job(_Store(), "acme/widgets")} + globex = {STATE_KEY: await read_for_job(_Store(), "globex/api")} + legacy = {STATE_KEY: await read_for_job(_Store(), "acme/legacy")} + assert job_section(acme, section).enabled is False + assert job_section(acme, section).classifier_guide == "Acme's own words." + assert job_section(globex, section) == section.defaults(), "acme's layer never reaches globex's jobs" + assert job_section(legacy, section).enabled is True, "a repository's layer outranks its organization's" + assert job_section(legacy, section).review_guide == "Acme reviews a cleanup by its tests alone." + + cleanup = {"coding_agent": "slopmaster", "work_nature": VERDICT} + assert _slop_review_section({**cleanup, **acme}) == "Acme reviews a cleanup by its tests alone.\n", ( + "the reviewer is told the guide its job read" + ) + assert _slop_review_section({**cleanup, **globex}).startswith("## This change is a slop cleanup\n") + + +def test_a_dungeon_is_drawn_from_its_repository_s_level() -> None: + at = datetime(2026, 11, 6, tzinfo=UTC) + meters = [ + {"repo": "acme/web", "area": "", "score": 0.5, "level": 3, "name": "Lasagna", "computed_at": at}, + {"repo": "globex/api", "area": "", "score": 0.5, "level": 3, "name": "Lasagna", "computed_at": at}, + {"repo": "initech/app", "area": "", "score": 0.9, "level": 5, "name": "Just The Slop", "computed_at": at}, + ] + levels = {"acme/web": 4, "globex/api": 2, "initech/app": None} + try: + drawn = dungeons([], {}, meters, min_level=levels.__getitem__) + except TypeError: + pytest.fail("the level a meter draws a dungeon from is fixed in the code") + found = {d["repo"] for d in drawn} + assert found == {"globex/api"}, "acme draws from level 4, globex from 2, and initech's meter makes none" + + +def test_a_stored_verdict_is_read_back_with_every_kind_it_kept() -> None: + """The watcher kept its stack's own kind (slop_policy.kinds); the proposals feed must not drop it.""" + from embry0.services.proposals import case_proposal + + stored = {"nature": "slop", "slop_score": 60, "kinds": ["naming", "dead_code"], "evidence": []} + row = { + "id": "hc-1", + "repo": "acme/web", + "title": "Clean up the names that lie", + "severity": "medium", + "occurrence_count": 3, + "detected_at": datetime(2026, 11, 6, tzinfo=UTC), + "created_at": datetime(2026, 11, 6, tzinfo=UTC), + "work_nature": stored, + } + proposal = case_proposal(row).to_dict() + assert proposal["slop_kinds"] == ["naming", "dead_code"], "an organization's own kind is read back as stored" + assert "(naming, dead code)" in proposal["details"] diff --git a/tests/unit/services/test_watcher.py b/tests/unit/services/test_watcher.py index 3fab2b4d..d9dceefd 100644 --- a/tests/unit/services/test_watcher.py +++ b/tests/unit/services/test_watcher.py @@ -656,7 +656,7 @@ def test_a_slop_proposal_carries_its_verdict_and_its_ticket_says_so(): "kinds": ["symptom_patch"], "evidence": [{"path": "svc/outbox.py", "why": "three retry wrappers around one insert"}], } - assert ticket_section(p["work_nature"]).startswith("## Slop, not a bug") + assert ticket_section(p["work_nature"], heading="Slop, not a bug").startswith("## Slop, not a bug") def test_a_proposal_its_verdict_calls_noise_files_nothing(): diff --git a/tests/unit/services/test_watcher_slop_policy.py b/tests/unit/services/test_watcher_slop_policy.py new file mode 100644 index 00000000..19a5f47e --- /dev/null +++ b/tests/unit/services/test_watcher_slop_policy.py @@ -0,0 +1,122 @@ +"""The watcher reads its stack's ``slop_policy`` (#169): what its analysis is told, its kinds, its ticket heading. + +Each analysis is told the stack's repository's ``classifier_guide`` and its +verdict keeps the kinds that policy lists; a slop finding's draft ticket +opens with the policy's ``ticket_heading``. The production watcher is built +with a reader of its own stack's policy, read on every analysis, so a change +needs no restart. +""" + +from __future__ import annotations + +import ast +import pathlib +from types import SimpleNamespace +from typing import Any + +import pytest + +import embry0.api.app as app_module +from embry0.settings.sections import CATALOG +from embry0.settings.store import build_snapshot +from tests.unit.services.test_watcher import _service + +LINES = ["[c] e1", "[c] e2", "[c] e3"] +SLOP_PROPOSAL = ( + "TITLE: Clean up the outbox retry wrappers\nSEVERITY: medium\nNATURE: slop\nSLOP_SCORE: 72\n" + "SLOP_KINDS: naming, symptom_patch\nSLOP_EVIDENCE: svc/outbox.py | three retry wrappers\n" + "BODY:\n## Evidence\nboom" +) + + +def _policy(**values: Any) -> Any: + section = CATALOG.get("slop_policy") + if section is None: + pytest.fail("the settings document has no slop_policy section: the watcher's words about slop are code") + model = section.validate(values) + + async def read() -> Any: + return model + + return read + + +@pytest.mark.asyncio +async def test_an_analysis_is_told_the_stack_s_guide_and_its_verdict_keeps_the_stack_s_kinds() -> None: + job = {"job_id": "job-w1", "status": "completed", "result_summary": SLOP_PROPOSAL} + issue = {"identifier": "ENG-1", "url": "https://tracker.example/ENG-1"} + svc = _service(lines=LINES, job_result=job, linear_issue=issue) + svc._slop_policy = _policy( + classifier_guide="Acme's slop: names that lie.\nKinds: naming.", + kinds=["naming"], + ticket_heading="Shape, not behaviour", + ) + out = await svc.run_once() + assert out["action"] == "proposed", out + task = svc._jobs.create.await_args.kwargs["task"] + assert "Acme's slop: names that lie.\nKinds: naming." in task, "the analysis is told the stack's guide" + assert "Tell slop apart from bugs" not in task + description = svc._filer.file_draft.await_args.kwargs["description"] + assert description.startswith("## Shape, not behaviour\n"), "the ticket opens with the stack's heading" + assert "slop score 72/100: naming)" in description, "the verdict keeps the stack's kinds, and only those" + + +@pytest.mark.asyncio +async def test_a_watcher_without_a_reader_reads_as_before() -> None: + job = {"job_id": "job-w1", "status": "completed", "result_summary": SLOP_PROPOSAL} + issue = {"identifier": "ENG-1", "url": "https://tracker.example/ENG-1"} + svc = _service(lines=LINES, job_result=job, linear_issue=issue) + out = await svc.run_once() + assert out["action"] == "proposed", out + assert "Tell slop apart from bugs" in svc._jobs.create.await_args.kwargs["task"] + description = svc._filer.file_draft.await_args.kwargs["description"] + assert description.startswith("## Slop, not a bug\n") + assert "slop score 72/100: symptom patch)" in description, "naming is no default kind" + + +@pytest.mark.asyncio +async def test_a_reader_that_fails_reads_the_defaults() -> None: + job = {"job_id": "job-w1", "status": "completed", "result_summary": SLOP_PROPOSAL} + svc = _service(lines=LINES, job_result=job, linear_issue={"identifier": "ENG-1"}) + + async def broken() -> Any: + raise RuntimeError("database is down") + + svc._slop_policy = broken + assert (await svc.run_once())["action"] == "proposed", "a policy that cannot be read never stops a tick" + + +def _watcher_service_call() -> ast.Call: + source = pathlib.Path(app_module.__file__).read_text() + for node in ast.walk(ast.parse(source)): + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "WatcherService": + return node + pytest.fail("no WatcherService(...) construction found in embry0/api/app.py") + + +def test_each_watcher_reads_its_own_stack_s_policy() -> None: + """The reader is optional in the signature: a watcher built without it reads the defaults for good.""" + passed = {kw.arg: ast.unparse(kw.value) for kw in _watcher_service_call().keywords if kw.arg} + assert passed.get("slop_policy") == "_watcher_slop_policy(app, stack.repo)", ( + f"each watcher must read its own stack's slop_policy, not {passed.get('slop_policy')!r}" + ) + + +@pytest.mark.asyncio +async def test_the_reader_reads_the_stack_s_repository_s_layers_now() -> None: + reader_for = getattr(app_module, "_watcher_slop_policy", None) + assert reader_for is not None, "app.py has no reader for a stack's slop_policy" + rows = [ + {"scope": "deployment", "data": {}, "generation": 1, "updated_at": None}, + {"scope": "organization:acme", "data": {"slop_policy": {"ticket_heading": "Acme"}}, "generation": 1}, + ] + + class Store: + async def snapshot(self) -> Any: + return build_snapshot(CATALOG, rows, token=(), log=False) + + app = SimpleNamespace(state=SimpleNamespace(settings=Store())) + assert (await reader_for(app, "acme/widgets")()).ticket_heading == "Acme" + assert (await reader_for(app, "globex/api")()).ticket_heading == "Slop, not a bug", "another owner's layer" + rows[1] = {"scope": "organization:acme", "data": {"slop_policy": {"ticket_heading": "Later"}}, "generation": 2} + assert (await reader_for(app, "acme/widgets")()).ticket_heading == "Later", "read on every call" diff --git a/tests/unit/settings/test_example.py b/tests/unit/settings/test_example.py index f7423b37..6c4683ee 100644 --- a/tests/unit/settings/test_example.py +++ b/tests/unit/settings/test_example.py @@ -29,8 +29,9 @@ def _comment_out(text: str) -> str: - """``text`` with every indented line that is not a comment written as one, as the example writes keys.""" - return "\n".join(_SET_LINE.sub(r"\1# \2", line) for line in text.split("\n")) + """``text`` with every indented line that is not a comment written as one, as the example writes keys: a + blank line of a text's block, which is the indent alone, as the bare mark.""" + return "\n".join(" #" if line == " " else _SET_LINE.sub(r"\1# \2", line) for line in text.split("\n")) def test_the_committed_example_is_what_the_sections_render() -> None: diff --git a/tests/unit/settings/test_slop_policy_section.py b/tests/unit/settings/test_slop_policy_section.py new file mode 100644 index 00000000..a4071e69 --- /dev/null +++ b/tests/unit/settings/test_slop_policy_section.py @@ -0,0 +1,306 @@ +"""The ``slop_policy`` section (#169): its defaults are what the code held, and a write is refused naming each key. + +The defaults are pinned against the values ``origin/main`` held before the +section, copied here as text, never imported from the code that replaced +them: the kinds, the classifier's and the reviewer's words, the route, the +ticket heading and the two Slop-O-Meter levels. +""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any + +import pytest + +from embry0.settings.document import DocumentInvalid, check +from embry0.settings.registry import Catalog +from embry0.settings.sections import CATALOG +from embry0.storage.seeds.pipeline_templates_builtin import BUILTIN_PIPELINE_TEMPLATES + +# ---- what origin/main held (embry0/services/slop.py, slop_meter/context.py at 62e07507) ---- + +OLD_KINDS = ( + "duplication", + "dead_code", + "half_wired", + "contradicting_layers", + "symptom_patch", + "misleading_docs", + "off_pattern", +) +OLD_TEMPLATE = "slop-cleanup" +OLD_CLASSIFIER_GUIDE = """\ +Tell slop apart from bugs: +- bug: the behavior differs from what the code, its docs or the ticket intend. + Fixing it changes behavior. +- slop: the behavior is what the code says, and the trouble is the code's + shape. Cleaning it keeps behavior. Kinds: duplication (the same logic in + more than one place), dead_code (paths, flags or files nothing reaches), + half_wired (a feature or abstraction started and not finished), + contradicting_layers (two layers that disagree about one thing), + symptom_patch (guards, retries, fallbacks or catch-alls stacked on a symptom + instead of its cause), misleading_docs (comments or docs that say what the + code does not do), off_pattern (code that ignores the repository's own way). +- noise: nothing to do. +A guard, retry or catch-all stacked on a symptom is slop when removing it and +fixing the shape keeps the outcome. When the code does the wrong thing (keeps +retrying what can never succeed, drops or duplicates data, answers wrongly), +it is a bug, even when sloppy code caused it: say bug, and name the slop in +the evidence. Do not call a real defect slop to make it look smaller. +slop_score (0-100) is how much of the trouble is shape rather than behavior: +0 for a pure bug, 100 for pure slop.""" +OLD_REVIEW_GUIDE = """\ +## This change is a slop cleanup + +It was judged slop, not a bug: it must keep behavior and improve shape. +Approve only when all of these hold, and request changes otherwise, saying which: +- behavior is unchanged: the tests pass, and no public behavior, API or output + moved (removing what nothing reaches is fine); +- the change removes or merges more than it adds, or is plainly simpler; +- it adds no new guard, retry, fallback, catch-all or flag to hide a symptom.""" +#: ``ticket_section`` opened every slop ticket with ``## Slop, not a bug``. +OLD_TICKET_HEADING = "Slop, not a bug" +#: ``METER_DUNGEON_LEVEL`` and the never-read ``LEAN_LEVEL`` (Sloppy Joe, which triage's prompt named). +OLD_DUNGEON_LEVEL = 2 +OLD_LEAN_LEVEL = 4 + + +def _section() -> Any: + section = CATALOG.get("slop_policy") + assert section is not None, "the settings document has no slop_policy section: slop routing is code" + return section + + +def test_the_defaults_are_what_the_code_held_before_the_section() -> None: + defaults = _section().defaults() + assert defaults.enabled is True, "every code job judged slop was routed" + assert defaults.route_template == OLD_TEMPLATE + assert defaults.min_score_to_route == 0, "a slop verdict of any score was routed" + assert defaults.kinds == OLD_KINDS + assert defaults.classifier_guide == OLD_CLASSIFIER_GUIDE, "the classifiers' words, to the byte" + assert defaults.review_guide == OLD_REVIEW_GUIDE, "the reviewer's words, to the byte" + assert defaults.ticket_heading == OLD_TICKET_HEADING + assert defaults.dungeon_min_level == OLD_DUNGEON_LEVEL + assert defaults.triage_lean_min_level == OLD_LEAN_LEVEL + + +def test_a_repository_sets_its_route_and_an_organization_everything() -> None: + section = _section() + assert section.repo_keys == {"enabled", "route_template", "min_score_to_route"}, ( + "a repository's layer, which a repository-scoped key writes, puts no text into a system prompt" + ) + assert section.org_keys == set(section.fields) + assert "xp" not in section.fields, "a cleanup's experience is the progression pack's outcomes.cleaned" + + +def _problems(raw: Mapping[str, Any], *, layer: str = "deployment", context: Mapping[str, Any] | None = None) -> dict: + _section() + try: + check( + CATALOG, + {"slop_policy": dict(raw)}, + stored={}, + layer=layer, + inherited={} if layer != "deployment" else None, + context=context, + ) + except DocumentInvalid as refused: + return {".".join(map(str, problem.loc)): problem.msg for problem in refused.problems} + pytest.fail(f"the {layer}'s write of {sorted(raw)} was taken") + + +def test_a_write_is_refused_naming_every_bad_key() -> None: + problems = _problems( + { + "kinds": [], + "classifier_guide": "x" * 4001, + "review_guide": " ", + "ticket_heading": "## Slop", + "route_template": " slop-cleanup", + "min_score_to_route": 101, + "triage_lean_min_level": 0, + "dungeon_min_level": 6, + } + ) + assert set(problems) == { + "slop_policy.kinds", + "slop_policy.classifier_guide", + "slop_policy.review_guide", + "slop_policy.ticket_heading", + "slop_policy.route_template", + "slop_policy.min_score_to_route", + "slop_policy.triage_lean_min_level", + "slop_policy.dungeon_min_level", + }, problems + assert "at least one kind" in problems["slop_policy.kinds"] + assert "4000" in problems["slop_policy.classifier_guide"] + assert "without its # marks" in problems["slop_policy.ticket_heading"] + + +@pytest.mark.parametrize( + ("raw", "words"), + [ + ({"kinds": ["Dead-Code"]}, "lowercase letters"), + ({"kinds": ["dead_code", "dead_code"]}, "each kind once"), + ({"kinds": [f"k{n}" for n in range(21)]}, "20"), + ({"classifier_guide": "a\x07b"}, "plain"), + ({"review_guide": "a‮b"}, "plain"), + ({"ticket_heading": "two\nlines"}, "one line"), + ({"ticket_heading": "x" * 81}, "80"), + ({"route_template": ""}, "1"), + ({"route_template": "x" * 101}, "100"), + ], +) +def test_each_rule_names_its_key(raw: dict[str, Any], words: str) -> None: + (key,) = raw + problems = _problems(raw) + assert words in problems[f"slop_policy.{key}"], problems + + +def test_a_repository_layer_sets_no_words_of_a_prompt() -> None: + _section() + problems = _problems( + {"classifier_guide": "Tell slop apart.", "kinds": ["naming"], "ticket_heading": "Shape"}, layer="repository" + ) + assert set(problems) == {"slop_policy.classifier_guide", "slop_policy.kinds", "slop_policy.ticket_heading"} + checked = check( + CATALOG, + {"slop_policy": {"enabled": False, "route_template": "quick-fix", "min_score_to_route": 50}}, + stored={}, + layer="repository", + inherited={}, + ) + assert checked.data["slop_policy"] == {"enabled": False, "route_template": "quick-fix", "min_score_to_route": 50} + + +def _templates(*, without: str = "") -> dict[str, Any]: + """The route template check as a write runs it, over the built-in templates and one of the operator's, less + ``without``.""" + from embry0.settings import write_context + + route_template_checker = getattr(write_context, "route_template_checker", None) + if route_template_checker is None: + pytest.fail("a settings write is not checked against the pipeline templates") + rows = [{"name": name, **seed} for name, seed in BUILTIN_PIPELINE_TEMPLATES.items() if name != without] + rows.append({"name": "tidy-up", **BUILTIN_PIPELINE_TEMPLATES["quick-fix"], "default_for_kind": None}) + return {"route_templates": route_template_checker(rows)} + + +@pytest.mark.parametrize( + ("template", "words"), + [ + ("nowhere", "no pipeline template named 'nowhere'"), + ("qa", "no pipeline template named 'qa'"), + ("research-default", "records its jobs as research jobs"), + ("kintsugi-trace", "records its jobs as analysis jobs"), + ("review-only", "has no coding step"), + ], +) +def test_a_write_names_a_template_that_takes_a_code_job(template: str, words: str) -> None: + problems = _problems({"route_template": template}, context=_templates()) + assert words in problems["slop_policy.route_template"], problems + + +@pytest.mark.parametrize("template", ["slop-cleanup", "quick-fix", "full-magnum-opus", "tidy-up"]) +def test_a_template_that_takes_a_code_job_is_accepted(template: str) -> None: + _section() + checked = check(CATALOG, {"slop_policy": {"route_template": template}}, stored={}, context=_templates()) + assert checked.data["slop_policy"]["route_template"] == template + + +def test_only_a_template_the_write_changes_is_checked() -> None: + """A template stored before (and since gone) never blocks a write of another key.""" + _section() + stored = {"slop_policy": {"route_template": "gone"}} + checked = check( + CATALOG, + {"slop_policy": {"route_template": "gone", "min_score_to_route": 40}}, + stored=stored, + context=_templates(), + ) + assert checked.data["slop_policy"] == {"route_template": "gone", "min_score_to_route": 40} + problems = _problems({"route_template": "gone"}, context=_templates()) + assert "no pipeline template named 'gone'" in problems["slop_policy.route_template"] + + +def test_offline_a_template_is_checked_for_its_shape_alone() -> None: + """``embry0 settings check`` has no deployment: a name it cannot know is left to the write.""" + checked = check(Catalog([_section()]), {"slop_policy": {"route_template": "nowhere"}}, stored={}) + assert checked.data["slop_policy"] == {"route_template": "nowhere"} + + +@pytest.mark.parametrize( + "write", + [{"slop_policy": {"enabled": True}}, {"slop_policy": None}, {}], + ids=["key-taken-out", "section-null", "section-left-out"], +) +def test_taking_route_template_out_checks_the_template_its_readers_then_route_to(write: dict[str, Any]) -> None: + """The deployment routes to tidy-up and slop-cleanup is gone: a write that leaves the default is refused.""" + _section() + stored = {"slop_policy": {"route_template": "tidy-up"}} + try: + check(CATALOG, write, stored=stored, context=_templates(without="slop-cleanup")) + except DocumentInvalid as refused: + named = {".".join(map(str, problem.loc)): problem.msg for problem in refused.problems} + else: + pytest.fail(f"{write} left the deployment routing to the default slop-cleanup, which is gone") + assert "no pipeline template named 'slop-cleanup'" in named.get("slop_policy.route_template", ""), ( + f"the write left the default slop-cleanup, which is gone, unchecked: {named}" + ) + + +def test_a_layer_that_takes_route_template_out_is_checked_for_what_it_inherits() -> None: + _section() + stored = {"slop_policy": {"route_template": "tidy-up"}} + try: + check( + CATALOG, + {"slop_policy": {"min_score_to_route": 10}}, + stored=stored, + layer="organization", + inherited={"slop_policy": {}}, + context=_templates(without="slop-cleanup"), + ) + except DocumentInvalid as refused: + problems = [(p.loc, p.type) for p in refused.problems] + else: + pytest.fail("the organization's layer took route_template out and now routes to slop-cleanup, which is gone") + assert problems == [(("slop_policy", "route_template"), "unavailable")], problems + checked = check( + CATALOG, + {"slop_policy": {"min_score_to_route": 10}}, + stored=stored, + layer="organization", + inherited={"slop_policy": {"route_template": "quick-fix"}}, + context=_templates(without="slop-cleanup"), + ) + assert checked.data["slop_policy"] == {"min_score_to_route": 10}, "it inherits quick-fix, which is there" + + +def test_a_write_that_leaves_the_route_as_it_was_is_taken_while_its_template_is_missing() -> None: + """slop-cleanup is gone and nothing names another: a write of another key, of this section or another, goes + through (jobs take their kind's default meanwhile).""" + _section() + for write in ({"slop_policy": {"min_score_to_route": 40}}, {"done_when": {"retries": 2}}): + try: + checked = check(CATALOG, write, stored={}, context=_templates(without="slop-cleanup")) + except DocumentInvalid as refused: + pytest.fail(f"{write} was refused for a template it does not change: {refused}") + assert checked.data == write + + +def test_naming_the_default_it_already_routes_to_is_taken_while_that_template_is_missing() -> None: + """A document that routes to the default writes it out explicitly: its readers' template does not change, so + the write is not judged by it, as a write of another key is not.""" + _section() + try: + checked = check( + CATALOG, + {"slop_policy": {"route_template": "slop-cleanup"}}, + stored={}, + context=_templates(without="slop-cleanup"), + ) + except DocumentInvalid as refused: + pytest.fail(f"writing out the default its readers already route to was refused: {refused}") + assert checked.data["slop_policy"] == {"route_template": "slop-cleanup"} diff --git a/tests/unit/workflows/test_policy_efficiency.py b/tests/unit/workflows/test_policy_efficiency.py index 0d96d331..70f84bb8 100644 --- a/tests/unit/workflows/test_policy_efficiency.py +++ b/tests/unit/workflows/test_policy_efficiency.py @@ -147,6 +147,8 @@ async def test_triage_system_prompt_moves_to_agent_definition(monkeypatch): from embry0.config import Embry0Config from embry0.orchestration.nodes.triage import build_triage_system_prompt from embry0.settings.sections.done_when import DONE_WHEN + from embry0.settings.sections.slop_meter import SLOP_METER + from embry0.settings.sections.slop_policy import SLOP_POLICY from embry0.workflows.issue_to_pr.nodes import triage_node # #465: the prompt is built per deployment configuration. No DB and no @@ -158,6 +160,8 @@ async def test_triage_system_prompt_moves_to_agent_definition(monkeypatch): tier: Embry0Config.model_fields[tier].default for tier in ("model_heavy", "model_medium", "model_light") }, ticket_sections=DONE_WHEN.defaults().ticket_sections, + slop_policy=SLOP_POLICY.defaults(), + level_names=SLOP_METER.defaults().level_names, ) captured = {} diff --git a/tests/unit/workflows/test_slop_route_policy.py b/tests/unit/workflows/test_slop_route_policy.py new file mode 100644 index 00000000..320a128b --- /dev/null +++ b/tests/unit/workflows/test_slop_route_policy.py @@ -0,0 +1,187 @@ +"""A code job judged slop is routed as its repository's ``slop_policy`` says (#169). + +plan_route reads the policy the job read when it started (``job_settings``): +whether it routes, from which score, to which template, and which kinds a +verdict may name. A job that read none routes as the code did before the +section: every slop verdict to ``slop-cleanup``. A route template that is +gone or cannot take a code job is logged and the job takes its kind's +default route; a slop route never fails a job. +""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import patch + +import pytest +import structlog + +from embry0.services.job_settings import STATE_KEY +from embry0.settings.sections import CATALOG +from embry0.storage.seeds.pipeline_templates_builtin import BUILTIN_PIPELINE_TEMPLATES +from embry0.workflows.issue_to_pr.plan_route import TemplateInvalidError, plan_route_node + +SLOP = {"nature": "slop", "slop_score": 72, "kinds": ["symptom_patch"], "evidence": []} + + +class _Templates: + """Template rows as the repository answers them: the built-ins named, and any of the operator's.""" + + def __init__(self, *names: str, **own: dict[str, Any]) -> None: + rows = [{"name": name, **BUILTIN_PIPELINE_TEMPLATES[name]} for name in names] + rows += [{"name": name, **row} for name, row in own.items()] + self._rows = {str(i): {"id": str(i), **row} for i, row in enumerate(rows)} + + async def list_all(self) -> list[dict[str, Any]]: + return [{k: v for k, v in r.items() if k != "graph_definition"} for r in self._rows.values()] + + async def get(self, template_id: str) -> dict[str, Any] | None: + return self._rows.get(template_id) + + +ALL = ("full-magnum-opus", "quick-fix", "slop-cleanup", "research-default") + + +def _state(policy: dict[str, Any] | None = None, *, verdict: Any = SLOP, **extra: Any) -> dict[str, Any]: + state: dict[str, Any] = { + "job_id": "job-1", + "repo": "acme/widgets", + "triage_decision": {"job_kind": "code", "work_nature": verdict, **extra.pop("decision", {})}, + **extra, + } + if policy is not None: + section = CATALOG.get("slop_policy") + if section is None: + pytest.fail("the settings document has no slop_policy section: a job judged slop is routed by code") + state[STATE_KEY] = {"slop_policy": section.validate(policy).model_dump(mode="json")} + return state + + +async def _route(state: dict[str, Any], templates: Any) -> dict[str, Any]: + with patch("embry0.workflows.issue_to_pr.plan_route.get_stream_writer", return_value=lambda _e: None): + return await plan_route_node(state, {"configurable": {"pipeline_templates_repo": templates}}) + + +@pytest.mark.asyncio +async def test_a_job_that_read_no_policy_routes_as_before() -> None: + """Passes before #169 by design: it pins what the code did.""" + out = await _route(_state(), _Templates(*ALL)) + assert (out["route_template_name"], out["coding_agent"]) == ("slop-cleanup", "slopmaster") + + +@pytest.mark.asyncio +async def test_the_policy_s_defaults_route_as_the_code_did() -> None: + out = await _route(_state({}), _Templates(*ALL)) + assert (out["route_template_name"], out["coding_agent"]) == ("slop-cleanup", "slopmaster") + + +@pytest.mark.asyncio +async def test_with_the_policy_off_a_job_judged_slop_takes_its_normal_route() -> None: + off = {"enabled": False} + out = await _route(_state(off), _Templates(*ALL)) + assert (out["route_template_name"], out["coding_agent"]) == ("full-magnum-opus", "developer"), ( + "slop_policy.enabled: false, so a job judged slop runs its kind's default" + ) + assert out["work_nature"] == SLOP, "the verdict is still recorded" + out = await _route(_state(off, decision={"pipeline_template": "quick-fix"}), _Templates(*ALL)) + assert out["route_template_name"] == "quick-fix", "and triage's own pick stands" + + +@pytest.mark.asyncio +async def test_a_verdict_below_the_threshold_keeps_its_route() -> None: + out = await _route(_state({"min_score_to_route": 80}), _Templates(*ALL)) + assert out["route_template_name"] == "full-magnum-opus", "72 is below 80" + out = await _route(_state({"min_score_to_route": 72}), _Templates(*ALL)) + assert out["route_template_name"] == "slop-cleanup", "72 reaches 72" + + +@pytest.mark.asyncio +async def test_a_job_judged_slop_runs_the_template_its_policy_names() -> None: + tidy = {**BUILTIN_PIPELINE_TEMPLATES["slop-cleanup"], "description": "the operator's own cleanup"} + out = await _route(_state({"route_template": "tidy-up"}), _Templates(*ALL, **{"tidy-up": tidy})) + assert (out["route_template_name"], out["coding_agent"]) == ("tidy-up", "slopmaster") + out = await _route( + _state({"route_template": "tidy-up"}, pipeline_template_request="quick-fix"), + _Templates(*ALL, **{"tidy-up": tidy}), + ) + assert out["route_template_name"] == "quick-fix", "a template a person named still wins" + + +@pytest.mark.asyncio +async def test_a_route_template_that_is_gone_or_unfit_falls_to_the_kind_default() -> None: + with structlog.testing.capture_logs() as logs: + gone = await _route(_state({"route_template": "tidy-up"}), _Templates(*ALL)) + assert gone["route_template_name"] == "full-magnum-opus" + assert any(e["event"] == "plan_route_template_not_found" and e.get("slop_route") for e in logs), logs + + with structlog.testing.capture_logs() as logs: + unfit = await _route(_state({"route_template": "research-default"}), _Templates(*ALL)) + assert unfit["route_template_name"] == "full-magnum-opus", "a research template never runs a code job" + assert unfit["job_kind"] == "code" + assert any(e["event"] == "plan_route_slop_template_unfit" for e in logs), logs + + +@pytest.mark.asyncio +async def test_a_broken_slop_route_never_fails_the_job() -> None: + """With no kind default, a person's invalid template fails the job; the policy's falls to the legacy plan.""" + graph = BUILTIN_PIPELINE_TEMPLATES["slop-cleanup"]["graph_definition"] + nodes = [*graph["nodes"], {"node_id": "stray", "agent_type": "no-such-agent", "position": {"x": 0, "y": 0}}] + broken = {**BUILTIN_PIPELINE_TEMPLATES["slop-cleanup"], "graph_definition": {**graph, "nodes": nodes}} + with structlog.testing.capture_logs() as logs: + try: + out = await _route(_state(), _Templates(**{"slop-cleanup": broken})) + except TemplateInvalidError: + pytest.fail("an invalid slop route with no code default failed the job") + assert any(e["event"] == "plan_route_template_invalid" for e in logs), "the template is invalid" + assert out["route_template_name"] is None and out["coding_agent"] == "developer", "the legacy plan runs" + + +@pytest.mark.asyncio +async def test_a_verdict_names_only_the_kinds_its_policy_lists() -> None: + verdict = {**SLOP, "kinds": ["naming", "symptom_patch", "dead-code"]} + out = await _route(_state({"kinds": ["naming", "dead_code"]}, verdict=verdict), _Templates(*ALL)) + assert out["work_nature"]["kinds"] == ["naming", "dead_code"], "an organization's own kind is kept" + out = await _route(_state({}, verdict=verdict), _Templates(*ALL)) + assert out["work_nature"]["kinds"] == ["symptom_patch", "dead_code"], "the default kinds, as before" + + +@pytest.mark.asyncio +async def test_a_job_pinned_to_the_full_pipeline_keeps_review_and_qa_when_judged_slop() -> None: + """#306: a job working a watcher-origin case runs review and QA, whatever route a slop policy names.""" + pinned = await _route(_state({"route_template": "quick-fix"}, pin_full_pipeline=True), _Templates(*ALL)) + steps = [step["agent_type"] for step in pinned["route_plan"]] + assert "qa" in steps and "reviewer" in steps, ( + f"a job pinned to the full pipeline judged slop ran {pinned['route_template_name']}: {steps}" + ) + assert pinned["route_template_name"] == "full-magnum-opus", "its kind's default route, which has both" + cleanup = await _route(_state({}, pin_full_pipeline=True), _Templates(*ALL)) + assert cleanup["route_template_name"] == "slop-cleanup", "the cleanup has review and QA, so it is taken" + unpinned = await _route(_state({"route_template": "quick-fix"}), _Templates(*ALL)) + assert unpinned["route_template_name"] == "quick-fix", "a job not pinned may take a lighter route" + + # A route with QA and no review step: the pinned job keeps both, so it takes its kind's default. + chain = ["triage", "developer", "qa", "output"] + checked = { + "description": "developer, then QA", + "graph_definition": { + "name": "checked-fix", + "nodes": [ + {"node_id": t, "agent_type": t, "label": t, "position": {"x": 240 * i, "y": 0}} + for i, t in enumerate(chain) + ], + "edges": [ + {"edge_id": f"e{i}", "source": a, "target": b, "edge_type": "flow"} + for i, (a, b) in enumerate(zip(chain, chain[1:], strict=False), start=1) + ], + "metadata": {}, + }, + } + with structlog.testing.capture_logs() as logs: + no_review = await _route( + _state({"route_template": "checked-fix"}, pin_full_pipeline=True), + _Templates(*ALL, **{"checked-fix": checked}), + ) + assert no_review["route_template_name"] == "full-magnum-opus", ( + f"a job pinned to the full pipeline judged slop ran {no_review['route_template_name']}, which has no review step" + ) + assert any(e["event"] == "plan_route_slop_template_unfit" and "review" in e["problem"] for e in logs), logs