diff --git a/alembic/versions/0092_note_verification.py b/alembic/versions/0092_note_verification.py new file mode 100644 index 0000000..52f73d1 --- /dev/null +++ b/alembic/versions/0092_note_verification.py @@ -0,0 +1,80 @@ +"""a note can carry its own check — verify_with, expires_when, verified_at +(milestone 317 step 1) + +Revision ID: 0092 +Revises: 0091 +Create Date: 2026-08-28 + +The sibling of 0090, which gave rules the same three columns. Same +distinction, one table over: + +A NORM is a decision — no truth value, and it changes only when its author +changes it, which they know they did. A CONSTRAINT is a fact about someone +else's software, and nobody is present when it goes false. + +Notes hold far more constraints than rules do, and hold them for longer. A +cross-project reference note asserting what a signing service does on a +duplicate upload, or how a forge numbers its CI runs, is believed by every +project that reads it, and there is nothing in the record that says when +anyone last looked. `note_supersessions` only fires once a human has read +the note, disagreed, and written the correction — which is the case where +the note was already believed. + +Three nullable columns: + +- `verify_with` — how to tell whether this is still true. A command, a path, + a URL, a query. Prose is allowed; something runnable is better. +- `expires_when` — the STATE under which it stops being true. Deliberately + not a date: constraints do not expire on a schedule, they expire when the + world underneath them moves. +- `verified_at` — when the check last passed. NULL means never checked, and + sorts FIRST in the sweep: unexamined outranks examined-long-ago. + +WHICH ROWS THESE ARE FOR. `notes` is one table holding notes, tasks, +snippets and processes, so these columns land on all of them. Only non-task, +non-snippet records are OFFERED them (milestone 317 decisions 1 and 2, gated +at the service in step 2): a task's decay is its status, and a snippet +already carries a richer, location-aware verdict in `data.verification`. The +columns exist on the other rows and stay null there; a gate that lives in +the schema would have meant a partial index or a CHECK across three columns +to express something the write path can say in two lines. + +All three optional, because most notes should set none of them — the whole +value of the sweep is that its output is short. A null `verify_with` is not +an omission; it is the honest marker of "this one is a decision, and there +is nothing to go and check." + +No CHECK constraint is involved, so rule 36 does not apply. Nothing is +backfilled: a migration cannot invent a check. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0092" +down_revision = "0091" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("notes", sa.Column("verify_with", sa.Text(), nullable=True)) + op.add_column("notes", sa.Column("expires_when", sa.Text(), nullable=True)) + op.add_column( + "notes", + sa.Column("verified_at", sa.DateTime(timezone=True), nullable=True), + ) + # No index, for 0090's reason — the sweep runs when a human asks, never on + # a request path — but the margin is thinner here and worth naming. `rules` + # is hundreds of rows; `notes` is thousands and grows with every session. + # + # Still a sequential scan's job at this size, and an index on + # (verified_at) filtered to `verify_with IS NOT NULL` would be maintained + # on every note write to serve one operator-initiated query. If step 3's + # live acceptance measures otherwise, add it there against a real plan + # rather than guessing here. + + +def downgrade() -> None: + op.drop_column("notes", "verified_at") + op.drop_column("notes", "expires_when") + op.drop_column("notes", "verify_with") diff --git a/alembic/versions/0093_rule_versions.py b/alembic/versions/0093_rule_versions.py new file mode 100644 index 0000000..1040196 --- /dev/null +++ b/alembic/versions/0093_rule_versions.py @@ -0,0 +1,83 @@ +"""rules gain an edit history — rule_versions (milestone 323 step 1) + +Revision ID: 0093 +Revises: 0092 +Create Date: 2026-08-29 + +The sibling `note_versions` has had for a long time. A note's every meaningful +edit is snapshotted, and the design-system note calls that history "the +changelog". A RULE — which binds behaviour on every session that loads it — +had nothing: an edit destroyed what it used to say, with no record anywhere. + +Rescoping rule 79 on 2026-08-29 is what surfaced it. The superseded statement +had to be hand-copied into a task log to survive the edit (#3237), which is +not a process, it is a person remembering. The more consequential record had +the weaker protection. + +Three things are deliberately NOT copied from note_versions, and each is a +guard that exists there for a reason that does not hold here: + +- **No pruning, and no MAX_VERSIONS.** That cap defends against note autosave + filling every slot. Rules have no autosave; every edit is a deliberate + update_rule. A rule is edited a handful of times in its life, and capping + invites losing the one edit somebody needed. +- **No pin columns.** `pin_kind`/`pin_label` exist so a note's version can + survive that pruning. With nothing pruning, a pin protects a row that was + never at risk. +- **No minimum interval.** 300 seconds between snapshots is also an autosave + defence; here it would only ever discard a second deliberate edit. + +`user_id` is the ACTOR rather than the owner, and is SET NULL rather than +CASCADE: deleting a user must not erase the history of the rules they edited. +The edit still happened and the rule still binds because of it. + +No CHECK constraint, so rule 36 does not apply. Nothing is backfilled — a +migration cannot invent the text a rule used to have, and inventing "the +current text, as of now" would be worse than an empty history, because it +would look like a record of an edit that never occurred. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0093" +down_revision = "0092" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "rule_versions", + sa.Column("id", sa.BigInteger(), primary_key=True), + sa.Column( + "rule_id", + sa.BigInteger(), + sa.ForeignKey("rules.id", ondelete="CASCADE"), + nullable=False, + ), + sa.Column( + "user_id", + sa.BigInteger(), + sa.ForeignKey("users.id", ondelete="SET NULL"), + nullable=True, + ), + sa.Column("title", sa.Text(), nullable=False, server_default=""), + sa.Column("statement", sa.Text(), nullable=False, server_default=""), + sa.Column("why", sa.Text(), nullable=True), + sa.Column("how_to_apply", sa.Text(), nullable=True), + sa.Column("when_to_apply", sa.Text(), nullable=True), + sa.Column("tier", sa.Text(), nullable=True), + sa.Column("verify_with", sa.Text(), nullable=True), + sa.Column("expires_when", sa.Text(), nullable=True), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + ) + # The only query this table serves is "the history of THIS rule, newest + # first" — unlike 0092's columns, which are read by an operator-initiated + # sweep over the whole set. Every read here is keyed on rule_id, so the + # index earns its write cost immediately rather than on a hunch. + op.create_index("ix_rule_versions_rule_id", "rule_versions", ["rule_id"]) + + +def downgrade() -> None: + op.drop_index("ix_rule_versions_rule_id", table_name="rule_versions") + op.drop_table("rule_versions") diff --git a/frontend/src/api/rulebooks.ts b/frontend/src/api/rulebooks.ts index f884999..9665f42 100644 --- a/frontend/src/api/rulebooks.ts +++ b/frontend/src/api/rulebooks.ts @@ -228,6 +228,49 @@ export async function unrelateRules(relationId: number): Promise { return apiDelete(`/api/rule-relations/${relationId}`); } +/** + * One entry in a rule's edit history. + * + * Each entry holds the text the edit REPLACED, not the text it introduced — + * so the newest entry is what the rule said before its most recent change, + * and what that change produced is the rule as it stands now. Read the other + * way round, every diff comes out backwards. + * + * The listing form omits the long fields; open one to get them. + */ +export interface RuleVersion { + id: number; + rule_id: number; + /** Who made the edit. Null when that account has since been deleted. */ + user_id: number | null; + title: string; + created_at: string; + statement?: string; + why?: string; + how_to_apply?: string; + when_to_apply?: string; + tier?: string; + verify_with?: string; + expires_when?: string; +} + +export async function listRuleVersions(ruleId: number): Promise { + const data = await apiGet<{ versions: RuleVersion[] }>( + `/api/rules/${ruleId}/versions`, + ); + return data.versions; +} + +export async function getRuleVersion( + ruleId: number, versionId: number, +): Promise { + return apiGet(`/api/rules/${ruleId}/versions/${versionId}`); +} + +// No restoreRuleVersion, deliberately (milestone 323). Putting an old wording +// back goes through updateRule, which snapshots what it replaces — so the +// undo stays visible in the history like any other edit. + export async function deleteRule(id: number): Promise { return apiDelete(`/api/rules/${id}`); } diff --git a/frontend/src/assets/editor-shared.css b/frontend/src/assets/editor-shared.css index 9637035..7cd8ff3 100644 --- a/frontend/src/assets/editor-shared.css +++ b/frontend/src/assets/editor-shared.css @@ -127,7 +127,7 @@ border: 1px solid var(--fs-error); border-radius: var(--fs-radius-sm); font-size: 0.85rem; - color: var(--fs-error); + color: var(--fs-error-fg); } .diff-view { border: 1px solid var(--fs-border-color); @@ -147,11 +147,11 @@ } .diff-delete { background: color-mix(in srgb, var(--fs-error) 12%, transparent); - color: var(--fs-error); + color: var(--fs-error-fg); } .diff-insert { background: color-mix(in srgb, var(--fs-success) 12%, transparent); - color: var(--fs-success); + color: var(--fs-success-fg); } .diff-equal { color: var(--fs-text-tertiary); diff --git a/frontend/src/assets/theme.css b/frontend/src/assets/theme.css index c4d7d9d..f174443 100644 --- a/frontend/src/assets/theme.css +++ b/frontend/src/assets/theme.css @@ -31,6 +31,7 @@ --fs-accent-faint: color-mix(in srgb, var(--fs-accent) 8%, transparent); /* The faintest accent wash */ --fs-accent-deep: color-mix(in srgb, var(--fs-accent) 70%, black); /* The accent, darkened */ --fs-accent-wash: color-mix(in srgb, var(--fs-accent) 22%, transparent); /* Heaviest accent tint */ + --fs-accent-fg: color-mix(in srgb, var(--fs-accent) 45%, var(--fs-text-primary)); /* Accent TEXT on an accent tint */ --fs-gradient-cta: linear-gradient(135deg, var(--fs-accent), var(--fs-accent-deep)); --fs-glow-cta: 0 2px 10px color-mix(in srgb, var(--fs-accent) 35%, transparent); --fs-glow-cta-hover: 0 4px 24px color-mix(in srgb, var(--fs-accent) 65%, transparent); @@ -102,8 +103,11 @@ /* semantic */ --fs-success: var(--fs-action-primary); + --fs-success-fg: color-mix(in srgb, var(--fs-success) 45%, var(--fs-text-primary)); /* Success TEXT on a success tint */ --fs-warning: #8B6F1E; + --fs-warning-fg: color-mix(in srgb, var(--fs-warning) 50%, var(--fs-text-primary)); /* Warning TEXT on a warning tint */ --fs-error: #C04A1F; + --fs-error-fg: color-mix(in srgb, var(--fs-error) 50%, var(--fs-text-primary)); /* Error TEXT on an error tint */ --fs-info: #3D5A6E; --fs-destructive: #6B2118; /* irreversible — deliberately not the error colour */ @@ -148,7 +152,9 @@ /* text */ --fs-text-primary: #E8E4D8; /* body, headings, labels — inverts by mode */ --fs-text-secondary: #C2BFB4; + --fs-text-secondary-fg: color-mix(in srgb, var(--fs-text-secondary) 90%, var(--fs-text-primary)); /* Secondary TEXT on a secondary tint (barely moves; no exceptions) */ --fs-text-tertiary: #9C9A92; + --fs-text-tertiary-fg: color-mix(in srgb, var(--fs-text-tertiary) 55%, var(--fs-text-primary)); /* Tertiary TEXT on a tertiary tint */ --fs-text-on-action: #E8E4D8; /* text on a filled colour — NOT mode-dependent */ /* type */ diff --git a/frontend/src/assets/viewer-shared.css b/frontend/src/assets/viewer-shared.css index 644fe1d..864c62d 100644 --- a/frontend/src/assets/viewer-shared.css +++ b/frontend/src/assets/viewer-shared.css @@ -25,7 +25,7 @@ border-color: var(--fs-accent); } .ctx-crumb-project { - color: var(--fs-accent); + color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); border: 1px solid color-mix(in srgb, var(--fs-accent) 30%, transparent); text-decoration: none; diff --git a/frontend/src/components/AppHeader.vue b/frontend/src/components/AppHeader.vue index f12f472..0363f46 100644 --- a/frontend/src/components/AppHeader.vue +++ b/frontend/src/components/AppHeader.vue @@ -206,7 +206,7 @@ router.afterEach(() => { background: var(--fs-accent-soft); } .nav-link.router-link-active { - color: var(--fs-accent); + color: var(--fs-accent-fg); font-weight: 500; background: color-mix(in srgb, var(--fs-accent) 25%, transparent); box-shadow: 0 0 16px color-mix(in srgb, var(--fs-accent) 30%, transparent); @@ -257,7 +257,7 @@ router.afterEach(() => { font-weight: 500; text-transform: uppercase; letter-spacing: 0.05em; - color: var(--fs-accent); + color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 15%, transparent); padding: 0.1rem 0.35rem; border-radius: var(--fs-radius-sm); diff --git a/frontend/src/components/DiffView.vue b/frontend/src/components/DiffView.vue index b2d4dde..7fb8754 100644 --- a/frontend/src/components/DiffView.vue +++ b/frontend/src/components/DiffView.vue @@ -137,12 +137,12 @@ function markerFor(type: DiffLine['type']): string { .diff-delete { background: color-mix(in srgb, var(--fs-error) 12%, transparent); - color: var(--fs-error); + color: var(--fs-error-fg); } .diff-insert { background: color-mix(in srgb, var(--fs-success) 12%, transparent); - color: var(--fs-success); + color: var(--fs-success-fg); } .diff-equal { diff --git a/frontend/src/components/HistoryPanel.vue b/frontend/src/components/HistoryPanel.vue index 0688a9c..56491bc 100644 --- a/frontend/src/components/HistoryPanel.vue +++ b/frontend/src/components/HistoryPanel.vue @@ -2,7 +2,7 @@ import { ref, computed, onMounted } from "vue"; import { apiGet, pinNoteVersion, unpinNoteVersion } from "@/api/client"; import DiffView from "@/components/DiffView.vue"; -import type { DiffLine } from "@/composables/useAssist"; +import { computeDiff, type DiffLine } from "@/utils/diff"; import { fmtStamp } from "@/utils/dateFormat"; interface NoteVersion { @@ -33,28 +33,8 @@ const loadingDetail = ref(false); const diff = computed(() => { if (!selectedVersion.value?.body) return []; - const a = props.currentBody; - const b = selectedVersion.value.body; - const aLines = a.split('\n'); - const bLines = b.split('\n'); - const m = aLines.length, n = bLines.length; - const dp: number[][] = Array.from({ length: m + 1 }, () => new Array(n + 1).fill(0)); - for (let i = m - 1; i >= 0; i--) - for (let j = n - 1; j >= 0; j--) - dp[i][j] = aLines[i] === bLines[j] - ? dp[i+1][j+1] + 1 - : Math.max(dp[i+1][j], dp[i][j+1]); - const result: DiffLine[] = []; - let i = 0, j = 0; - while (i < m && j < n) { - if (aLines[i] === bLines[j]) { result.push({ type: 'equal', text: aLines[i++] }); j++; } - else if (dp[i+1][j] >= dp[i][j+1]) result.push({ type: 'delete', text: aLines[i++] }); - else result.push({ type: 'insert', text: bLines[j++] }); - } - while (i < m) result.push({ type: 'delete', text: aLines[i++] }); - while (j < n) result.push({ type: 'insert', text: bLines[j++] }); - return result; + return computeDiff(props.currentBody, selectedVersion.value.body); }); async function loadVersions() { diff --git a/frontend/src/components/InlineAssistPanel.vue b/frontend/src/components/InlineAssistPanel.vue index 313008b..60e89a5 100644 --- a/frontend/src/components/InlineAssistPanel.vue +++ b/frontend/src/components/InlineAssistPanel.vue @@ -227,11 +227,11 @@ const markers: Record = { .iap-diff-equal { color: var(--fs-text-tertiary); } .iap-diff-delete { background: color-mix(in srgb, var(--fs-error) 10%, transparent); - color: var(--fs-error); + color: var(--fs-error-fg); } .iap-diff-insert { background: color-mix(in srgb, var(--fs-success) 10%, transparent); - color: var(--fs-success); + color: var(--fs-success-fg); } .iap-diff-marker { diff --git a/frontend/src/components/MarkdownToolbar.vue b/frontend/src/components/MarkdownToolbar.vue index 7ba034a..8741855 100644 --- a/frontend/src/components/MarkdownToolbar.vue +++ b/frontend/src/components/MarkdownToolbar.vue @@ -156,7 +156,7 @@ const groups = [ .md-btn.active { background: color-mix(in srgb, var(--fs-accent) 14%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); box-shadow: 0 0 0 1px color-mix(in srgb, var(--fs-accent) 35%, transparent); } diff --git a/frontend/src/components/NoteSweepPane.vue b/frontend/src/components/NoteSweepPane.vue new file mode 100644 index 0000000..cd18eb9 --- /dev/null +++ b/frontend/src/components/NoteSweepPane.vue @@ -0,0 +1,198 @@ + + + + + diff --git a/frontend/src/components/SystemsSection.vue b/frontend/src/components/SystemsSection.vue index 6bfbbf3..d3411f7 100644 --- a/frontend/src/components/SystemsSection.vue +++ b/frontend/src/components/SystemsSection.vue @@ -679,7 +679,7 @@ async function confirmDelete() { font-weight: 500; background: color-mix(in srgb, var(--fs-accent) 12%, transparent); border: 1px solid color-mix(in srgb, var(--fs-accent) 30%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); border-radius: 999px; padding: 0.05rem 0.45rem; flex-shrink: 0; @@ -689,7 +689,7 @@ async function confirmDelete() { font-weight: 500; text-transform: uppercase; letter-spacing: 0.04em; - color: var(--fs-text-tertiary); + color: var(--fs-text-tertiary-fg); background: color-mix(in srgb, var(--fs-text-tertiary) 12%, transparent); border-radius: 999px; padding: 0.05rem 0.45rem; diff --git a/frontend/src/components/TagInput.vue b/frontend/src/components/TagInput.vue index 986225b..00a4403 100644 --- a/frontend/src/components/TagInput.vue +++ b/frontend/src/components/TagInput.vue @@ -168,7 +168,7 @@ function focusInput() { border-radius: 999px; background: color-mix(in srgb, var(--fs-accent) 15%, transparent); border: 1px solid var(--fs-accent); - color: var(--fs-accent); + color: var(--fs-accent-fg); font-size: 0.8rem; white-space: nowrap; } diff --git a/frontend/src/components/VersionHistorySection.vue b/frontend/src/components/VersionHistorySection.vue index 6c7cc16..fb344c6 100644 --- a/frontend/src/components/VersionHistorySection.vue +++ b/frontend/src/components/VersionHistorySection.vue @@ -2,7 +2,7 @@ import { ref, computed } from "vue"; import { apiGet } from "@/api/client"; import DiffView from "@/components/DiffView.vue"; -import type { DiffLine } from "@/composables/useAssist"; +import { computeDiff, type DiffLine } from "@/utils/diff"; interface NoteVersion { id: number; @@ -31,25 +31,7 @@ const loadingDetail = ref(false); const diff = computed(() => { if (!selectedVersion.value?.body) return []; - const aLines = props.currentBody.split("\n"); - const bLines = selectedVersion.value.body.split("\n"); - const m = aLines.length, n = bLines.length; - const dp: number[][] = Array.from({ length: m + 1 }, () => new Array(n + 1).fill(0)); - for (let i = m - 1; i >= 0; i--) - for (let j = n - 1; j >= 0; j--) - dp[i][j] = aLines[i] === bLines[j] - ? dp[i + 1][j + 1] + 1 - : Math.max(dp[i + 1][j], dp[i][j + 1]); - const result: DiffLine[] = []; - let i = 0, j = 0; - while (i < m && j < n) { - if (aLines[i] === bLines[j]) { result.push({ type: "equal", text: aLines[i++] }); j++; } - else if (dp[i + 1][j] >= dp[i][j + 1]) result.push({ type: "delete", text: aLines[i++] }); - else result.push({ type: "insert", text: bLines[j++] }); - } - while (i < m) result.push({ type: "delete", text: aLines[i++] }); - while (j < n) result.push({ type: "insert", text: bLines[j++] }); - return result; + return computeDiff(props.currentBody, selectedVersion.value.body); }); function formatDate(iso: string): string { diff --git a/frontend/src/components/WorkspaceNoteEditor.vue b/frontend/src/components/WorkspaceNoteEditor.vue index d54fb55..a3462a4 100644 --- a/frontend/src/components/WorkspaceNoteEditor.vue +++ b/frontend/src/components/WorkspaceNoteEditor.vue @@ -548,7 +548,7 @@ defineExpose({ reload: loadProjectNotes }); .note-tag-pill { font-size: 0.58rem; - color: var(--fs-accent); + color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); border-radius: 999px; padding: 0 0.3rem; @@ -645,7 +645,7 @@ defineExpose({ reload: loadProjectNotes }); .btn-tag-suggestion.applied { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); border-color: var(--fs-accent); - color: var(--fs-accent); + color: var(--fs-accent-fg); } .link-suggest-strip { diff --git a/frontend/src/components/WorkspaceTaskPanel.vue b/frontend/src/components/WorkspaceTaskPanel.vue index f8b101e..d98bdae 100644 --- a/frontend/src/components/WorkspaceTaskPanel.vue +++ b/frontend/src/components/WorkspaceTaskPanel.vue @@ -424,8 +424,8 @@ defineExpose({ reload: loadAll }); border-radius: 10px; text-transform: capitalize; } -.ms-status-active { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); color: var(--fs-accent); } -.ms-status-completed { background: color-mix(in srgb, var(--fs-success) 15%, transparent); color: var(--fs-success); } +.ms-status-active { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); color: var(--fs-accent-fg); } +.ms-status-completed { background: color-mix(in srgb, var(--fs-success) 15%, transparent); color: var(--fs-success-fg); } .task-items { list-style: none; @@ -516,8 +516,8 @@ defineExpose({ reload: loadAll }); user-select: none; margin-left: auto; } -.status-cycler.status-in_progress { border-color: var(--fs-accent); color: var(--fs-accent); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); } -.status-cycler.status-done { border-color: var(--fs-success); color: var(--fs-success); background: color-mix(in srgb, var(--fs-success) 10%, transparent); } +.status-cycler.status-in_progress { border-color: var(--fs-accent); color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); } +.status-cycler.status-done { border-color: var(--fs-success); color: var(--fs-success-fg); background: color-mix(in srgb, var(--fs-success) 10%, transparent); } .btn-edit-task { margin-left: 0.25rem; } .btn-edit-task:hover { text-decoration: underline; } diff --git a/frontend/src/components/rules/RuleEditorSlideOver.vue b/frontend/src/components/rules/RuleEditorSlideOver.vue index 26523bf..22f54c4 100644 --- a/frontend/src/components/rules/RuleEditorSlideOver.vue +++ b/frontend/src/components/rules/RuleEditorSlideOver.vue @@ -3,6 +3,7 @@ import { computed, ref, watch, onMounted } from "vue"; import { useRulebooksStore } from "@/stores/rulebooks"; import { useCanonicalSystemsStore } from "@/stores/canonicalSystems"; import type { RuleTier } from "@/api/rulebooks"; +import RuleHistoryPanel from "@/components/rules/RuleHistoryPanel.vue"; const props = defineProps<{ ruleId: number | null; topicId: number | null }>(); const emit = defineEmits<{ close: [] }>(); @@ -256,6 +257,17 @@ watch(() => props.ruleId, load); How to apply + +
+ + +
+ +
@@ -678,7 +726,7 @@ onUnmounted(() => assist.clearSelection()); flex-direction: column; } -.sb-select, .sb-input { +.sb-select, .sb-input, .sb-textarea { width: 100%; padding: 5px 8px; border-radius: var(--fs-radius-sm); @@ -690,9 +738,27 @@ onUnmounted(() => assist.clearSelection()); outline: none; transition: border-color 0.15s; } -.sb-select:focus, .sb-input:focus { +.sb-select:focus, .sb-input:focus, .sb-textarea:focus { border-color: var(--fs-accent); } +.sb-textarea { + resize: vertical; + line-height: 1.4; + box-sizing: border-box; +} +/* No red/amber ramp, matching RuleSweepPane: a colour scale would restate the + sweep's ordering and force an invented "stale after N days" threshold. + "Never" is marked because it is categorically different from a date, not a + worse one — it means nobody has ever confirmed the claim. */ +.check-age { + font-size: 0.7rem; + color: var(--fs-text-secondary); + font-variant-numeric: tabular-nums; +} +.check-age.unchecked { + font-style: italic; + color: var(--fs-text-tertiary); +} /* Link Suggestions */ .link-suggest-field { gap: 0.4rem; } diff --git a/frontend/src/views/NoteViewerView.vue b/frontend/src/views/NoteViewerView.vue index 9c09f32..9f65ae4 100644 --- a/frontend/src/views/NoteViewerView.vue +++ b/frontend/src/views/NoteViewerView.vue @@ -416,7 +416,7 @@ async function convertToTask() { } .badge-note { background: color-mix(in srgb, var(--fs-accent) 12%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); border: 1px solid color-mix(in srgb, var(--fs-accent) 25%, transparent); } .badge-task { diff --git a/frontend/src/views/ProjectView.vue b/frontend/src/views/ProjectView.vue index 512d677..4901c3d 100644 --- a/frontend/src/views/ProjectView.vue +++ b/frontend/src/views/ProjectView.vue @@ -1287,8 +1287,8 @@ async function confirmDelete() { .stat-todo { background: color-mix(in srgb, var(--fs-text-tertiary) 8%, transparent); color: var(--fs-text-secondary); border-color: var(--fs-border-color); } .stat-inprogress { background: color-mix(in srgb, #3b82f6 10%, transparent); color: #3b82f6; border-color: color-mix(in srgb, #3b82f6 28%, transparent); } -.stat-done { background: color-mix(in srgb, var(--fs-success) 10%, transparent); color: var(--fs-success); border-color: color-mix(in srgb, var(--fs-success) 28%, transparent); } -.stat-notes { background: color-mix(in srgb, var(--fs-accent) 8%, transparent); color: var(--fs-accent); border-color: color-mix(in srgb, var(--fs-accent) 22%, transparent); } +.stat-done { background: color-mix(in srgb, var(--fs-success) 10%, transparent); color: var(--fs-success-fg); border-color: color-mix(in srgb, var(--fs-success) 28%, transparent); } +.stat-notes { background: color-mix(in srgb, var(--fs-accent) 8%, transparent); color: var(--fs-accent-fg); border-color: color-mix(in srgb, var(--fs-accent) 22%, transparent); } /* ── Pattern-library coverage card ───────────────────────────── */ .coverage-card { @@ -1471,7 +1471,7 @@ async function confirmDelete() { .tab-btn.active .tab-count { background: color-mix(in srgb, var(--fs-accent) 12%, transparent); border-color: color-mix(in srgb, var(--fs-accent) 30%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); } /* ── Tasks view ──────────────────────────────────────────────── */ @@ -1707,7 +1707,7 @@ async function confirmDelete() { border-radius: 3px; margin-left: auto; } -.col-add-btn:hover { color: var(--fs-accent); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); } +.col-add-btn:hover { color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 10%, transparent); } .kanban-cards { display: flex; flex-direction: column; gap: 0.3rem; } diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 2d94d75..360447a 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -2732,7 +2732,7 @@ async function deleteUser(userId: number) { background: var(--fs-surface-raised); } .sidebar-item.active { - color: var(--fs-accent); + color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 8%, transparent); border-left-color: var(--fs-accent); font-weight: 500; @@ -3099,7 +3099,7 @@ async function deleteUser(userId: number) { border-radius: var(--fs-radius-sm); } .role-admin { - color: var(--fs-accent); + color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 15%, transparent); } .role-user { @@ -3179,9 +3179,9 @@ async function deleteUser(userId: number) { text-transform: uppercase; letter-spacing: 0.05em; padding: 0.1rem 0.35rem; border-radius: var(--fs-radius-sm); } -.cat-audit { color: var(--fs-accent); background: color-mix(in srgb, var(--fs-accent) 15%, transparent); } -.cat-usage { color: var(--fs-success); background: color-mix(in srgb, var(--fs-success) 15%, transparent); } -.cat-error { color: var(--fs-error); background: color-mix(in srgb, var(--fs-error) 15%, transparent); } +.cat-audit { color: var(--fs-accent-fg); background: color-mix(in srgb, var(--fs-accent) 15%, transparent); } +.cat-usage { color: var(--fs-success-fg); background: color-mix(in srgb, var(--fs-success) 15%, transparent); } +.cat-error { color: var(--fs-error-fg); background: color-mix(in srgb, var(--fs-error) 15%, transparent); } .method-tag { display: inline-block; font-size: 0.65rem; font-weight: 500; font-family: monospace; @@ -3346,8 +3346,8 @@ async function deleteUser(userId: number) { padding: 0.15rem 0.4rem; border-radius: 4px; } -.role-owner { background: color-mix(in srgb, var(--fs-warning) 15%, transparent); color: var(--fs-warning); } -.role-member { background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); color: var(--fs-text-tertiary); } +.role-owner { background: color-mix(in srgb, var(--fs-warning) 15%, transparent); color: var(--fs-warning-fg); } +.role-member { background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); color: var(--fs-text-tertiary-fg); } .members-empty { color: var(--fs-text-tertiary); @@ -3528,7 +3528,7 @@ async function deleteUser(userId: number) { .day-btn.active { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); border-color: var(--fs-accent); - color: var(--fs-accent); + color: var(--fs-accent-fg); font-weight: 500; } diff --git a/frontend/src/views/SharedWithMeView.vue b/frontend/src/views/SharedWithMeView.vue index 13d7e75..fe5720b 100644 --- a/frontend/src/views/SharedWithMeView.vue +++ b/frontend/src/views/SharedWithMeView.vue @@ -242,9 +242,9 @@ onMounted(async () => { border-radius: 4px; white-space: nowrap; } -.perm-viewer { background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); color: var(--fs-text-tertiary); } -.perm-editor { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); color: var(--fs-accent); } -.perm-admin { background: color-mix(in srgb, var(--fs-warning) 15%, transparent); color: var(--fs-warning); } +.perm-viewer { background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); color: var(--fs-text-tertiary-fg); } +.perm-editor { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); color: var(--fs-accent-fg); } +.perm-admin { background: color-mix(in srgb, var(--fs-warning) 15%, transparent); color: var(--fs-warning-fg); } .empty-msg { margin: 0; diff --git a/frontend/src/views/SnippetDetailView.vue b/frontend/src/views/SnippetDetailView.vue index 08657dc..f7d2720 100644 --- a/frontend/src/views/SnippetDetailView.vue +++ b/frontend/src/views/SnippetDetailView.vue @@ -288,7 +288,7 @@ async function confirmDelete() { font-family: var(--fs-font-mono); font-size: 0.82rem; background: color-mix(in srgb, var(--fs-accent) 12%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); padding: 0.08rem 0.35rem; border-radius: var(--fs-radius-sm); word-break: break-all; diff --git a/frontend/src/views/SnippetListView.vue b/frontend/src/views/SnippetListView.vue index 4a739ad..99541e9 100644 --- a/frontend/src/views/SnippetListView.vue +++ b/frontend/src/views/SnippetListView.vue @@ -710,7 +710,7 @@ function usageTitle(s: SnippetListItem): string { flex-shrink: 0; white-space: nowrap; background: color-mix(in srgb, var(--fs-accent) 15%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); } .snippet-when { @@ -739,7 +739,7 @@ function usageTitle(s: SnippetListItem): string { border-radius: 4px; white-space: nowrap; background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); - color: var(--fs-text-tertiary); + color: var(--fs-text-tertiary-fg); } .dup-action { @@ -754,7 +754,7 @@ function usageTitle(s: SnippetListItem): string { border-radius: 4px; white-space: nowrap; background: color-mix(in srgb, var(--fs-error) 15%, transparent); - color: var(--fs-error); + color: var(--fs-error-fg); } .usage-tag { @@ -764,14 +764,14 @@ function usageTitle(s: SnippetListItem): string { white-space: nowrap; font-variant-numeric: tabular-nums; background: color-mix(in srgb, var(--fs-text-tertiary) 15%, transparent); - color: var(--fs-text-tertiary); + color: var(--fs-text-tertiary-fg); } /* Dead weight is a nudge, not an error — it warns in the warning colour rather than the danger one, because the record isn't broken, just unearned. */ .usage-tag.usage-dead { background: color-mix(in srgb, var(--fs-warning) 18%, transparent); - color: var(--fs-warning); + color: var(--fs-warning-fg); } /* Header + select-mode */ diff --git a/plugin/.claude-plugin/plugin.json b/plugin/.claude-plugin/plugin.json index 7f68734..eafde1b 100644 --- a/plugin/.claude-plugin/plugin.json +++ b/plugin/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "scribe", "description": "Scribe system-of-record for Claude Code: MCP tools over your notes/tasks/projects/rules, a session-start push channel that surfaces your always-on rules + active-project context, process-skills (writing-plans, systematic-debugging, verification, brainstorming, reusing-code), and your saved Scribe Processes auto-surfaced as skills (/scribe:sync). Replaces superpowers + file-memory with one app-backed plugin.", - "version": "0.1.47", + "version": "0.1.48", "author": { "name": "Bryan Van Deusen" }, diff --git a/plugin/hooks/scribe_after_write.sh b/plugin/hooks/scribe_after_write.sh index 52dcfd4..10857d2 100644 --- a/plugin/hooks/scribe_after_write.sh +++ b/plugin/hooks/scribe_after_write.sh @@ -175,6 +175,16 @@ while IFS= read -r rel_path; do derive_seen=$(tr '\n' ',' < "$derivefile" 2>/dev/null | sed 's/,$//' | jq -sRr '@uri' 2>/dev/null) || derive_seen="" [ -n "$derive_seen" ] && derive_exclude_q="&exclude_derive=${derive_seen}" fi + # The rules marker the SessionStart hook stored, handed back so the server + # can say whether those rules moved since (milestone 323). Nothing stored + # means nothing sent, which the server reads as silence rather than as a + # mismatch — an install that never reached /api/plugin/context must not + # start claiming its rules changed. + etag_q="" + if [ -f "$state_dir/${safe_sid}.rules_etag" ]; then + held=$(jq -sRr '@uri' < "$state_dir/${safe_sid}.rules_etag" 2>/dev/null) || held="" + [ -n "$held" ] && etag_q="&rules_etag=${held}" + fi if [ -n "$path_enc" ]; then # 8s, not the pre-write hook's 5: this hook runs AFTER the tool, so it # gates nothing the session is waiting on, and the first prior-art call @@ -184,7 +194,7 @@ while IFS= read -r rel_path; do reached=1 body=$(curl -fsS --max-time 8 \ -H "Authorization: Bearer ${token}" \ - "${url%/}/api/plugin/prior-art?path=${path_enc}&code=${code_enc}${repo_q}${exclude_q}${sync_exclude_q}${derive_exclude_q}${shapes_q}" 2>/dev/null) || { body=""; reached=0; } + "${url%/}/api/plugin/prior-art?path=${path_enc}&code=${code_enc}${repo_q}${exclude_q}${sync_exclude_q}${derive_exclude_q}${shapes_q}${etag_q}" 2>/dev/null) || { body=""; reached=0; } # A call that was owed and didn't come back is said, once per outage # (#2932) — shared marker with the pre-write hook, so one outage is one # line however the code was written. diff --git a/plugin/hooks/scribe_session_context.sh b/plugin/hooks/scribe_session_context.sh index 04aad2a..77b2ab3 100755 --- a/plugin/hooks/scribe_session_context.sh +++ b/plugin/hooks/scribe_session_context.sh @@ -109,6 +109,28 @@ if [ -n "$url" ] && [ -n "$token" ] && command -v curl >/dev/null 2>&1; then -H "Authorization: Bearer ${token}" \ "${url%/}/api/plugin/context${q}" 2>/dev/null) || body="" [ -n "$body" ] && dyn=$(printf '%s' "$body" | jq -r '.context // empty' 2>/dev/null) + # Stash the rules marker for the write-path hook (milestone 323). THIS is + # where it has to be captured: the model receives one from + # list_always_on_rules too, but a hook cannot see an MCP tool's result. Stored + # under the same state dir the prior-art hook already uses, keyed by session, + # so "changed since" means since THIS session loaded its rules. + # + # Written on `compact` as well as `startup`, and that is correct rather than + # convenient: a compact tells the session to re-pull its rules, so the marker + # should describe the set it is about to hold. It is also why this cannot + # cover the compaction case — see the table in services/plugin_context.py. + if [ -n "$body" ]; then + etag=$(printf '%s' "$body" | jq -r '.rules_etag // empty' 2>/dev/null) || etag="" + if [ -n "$etag" ]; then + sid=$(printf '%s' "$event" | jq -r '.session_id // empty' 2>/dev/null) || sid="" + safe_sid=$(printf '%s' "${sid:-nosession}" | tr -c 'A-Za-z0-9._-' '_') + etag_dir="${TMPDIR:-/tmp}/scribe-priorart" + # Best-effort throughout: a marker that cannot be stored costs a hint, + # never the session. + mkdir -p "$etag_dir" 2>/dev/null \ + && printf '%s' "$etag" > "$etag_dir/${safe_sid}.rules_etag" 2>/dev/null || true + fi + fi [ -z "$dyn" ] && status="> ⚠️ Scribe: live rules/project context could not be loaded this session (instance unreachable or request failed). The standing guidance above still applies — pull rules with \`list_always_on_rules()\` and project context with \`enter_project()\` as needed." elif [ -n "$url" ] && [ -z "$token" ]; then status="> ⚠️ Scribe: live context disabled this session — the API key is not configured (Scribe base URL is). Set it with \`/plugin\` → Scribe → configure, or export SCRIBE_TOKEN. Tools still work; pull rules with \`list_always_on_rules()\` and project context with \`enter_project()\`." diff --git a/plugin/skills/using-scribe/SKILL.md b/plugin/skills/using-scribe/SKILL.md index 9d1faa5..9a81f64 100644 --- a/plugin/skills/using-scribe/SKILL.md +++ b/plugin/skills/using-scribe/SKILL.md @@ -96,7 +96,25 @@ Two constraints on *how* that's achieved: not restraint. Only a record genuinely about no particular area goes untagged. -8. **State updates in place; chronicles don't.** A dev-log records what +8. **Name the record, never just its number.** Whenever you refer to a Scribe + record — in a message to the operator, a commit message, a task body, a + work-log — write the id *and* its title: `#3244 "the staleness signal"`, + `milestone 323 "rule versioning"`. Not `#3244`. + + You have the record open; the operator does not. A bare id reads as + complete to you and as homework to them — they have to look it up to know + what their own conversation is about, or guess. Scribe's own duplicate gate + already writes `id 412: "debounce helper"` for exactly this reason; match + it everywhere else. + + The first mention in a message carries the title; later mentions of the + same record can use the bare id. If you don't know the title, look it up + before citing the number — an id you can't name is one you haven't checked. + This matters most in the places read later by someone with even less + context than the operator has now: commit messages, task bodies, and any + record that cites another. + +9. **State updates in place; chronicles don't.** A dev-log records what *happened* — write it once, never rewrite it. A durable finding (how a subsystem works, a measured number) lives in that System's **reference note** ("«System» — reference"), which you UPDATE as facts change — safe, @@ -106,6 +124,48 @@ Two constraints on *how* that's achieved: re-measurement, a reversed decision), pass the old id in `supersedes` so the stale record is demoted and labelled rather than left competing. +10. **A few notes assert a FACT, and those can carry their own check.** + Supersession only fires once somebody has read a note and disagreed — which + is the case where it was already believed. A note asserting something about + *someone else's* software — what a service does on a duplicate upload, how a + forge numbers its CI runs, what an updater compares — can instead carry + `verify_with` (how to check it) and `expires_when` (the STATE that ends it: + "when the forge numbers runs per workflow", never "in six months"). + `notes_due_for_verification` lists them least-recently-confirmed first, with + never-checked at the top; `mark_note_verified` records what you found, and + `still_true=False` deliberately writes nothing — a note whose check failed + is wrong rather than in a state worth recording, so it keeps its place. + + **The test is one question: could this note become false without anyone + editing it?** If no, leave both fields empty. That is the normal case, and + an empty `verify_with` is the positive marker for "this is a decision, there + is nothing to go and check" — not an unfinished record. The sweep is only + worth reading while almost nothing is on it, so a check added out of + tidiness costs the whole surface, not just that note. + + **The sharper form of the same test: is the thing this note describes yours + to change?** If yes it is a decision — editing your own software is how it + changes, and you will know you did it. Measured against a real corpus, every + note that earned a check was about somebody ELSE's software: a signing + service, a forge, a hub, an SDK, a model, a dependency set. + + **Three that look like candidates and are not:** + - **Resume pointers and "current state" notes.** They go stale fastest of + anything, which is exactly why they tempt — but the cure is to update or + delete them, not to schedule a check. A sweep full of pointers is a sweep + nobody reads. + - **Measurements of your own system.** They go false because you changed + something, and you knew. A measurement earns a check only when what it + measures is outside your control. + - **A decision that RESTS on somebody else's behaviour.** The decision is + still a decision. Put the check on the note asserting the fact, and link + the decision to it. + + Not for tasks — a task's decay is its status, and a done issue records what + happened rather than asserting something that can go false. Not for snippets + either: `verify_snippet` compares the recorded location and code against the + repo, which is richer and already wired to drift detection. + ## Stay inside the active project's scope Once a project is in scope — you called `enter_project`, or the working repo is diff --git a/scripts/check_design_tokens.py b/scripts/check_design_tokens.py index 6670c7e..f6db544 100644 --- a/scripts/check_design_tokens.py +++ b/scripts/check_design_tokens.py @@ -119,15 +119,20 @@ def same_hue_text_on_tint(css: str) -> tuple[list[str], list[str]]: dangerous of the two — it does not even name a `-bg` token, so nothing about it looks like the pattern until you measure it. - Returned separately because they are at different stages. The token form - is CLEAN and therefore gates. The inline form has a live backlog (48 sites - when this split was written, 26 of them --fs-accent), so it reports with a - count: a gate nobody can satisfy today gets switched off, and then it - guards nothing. + Returned separately because they were paid down separately — the token + form first (7 badge pairs), then the inline form (46 sites across 18 + files, 26 of them --fs-accent). Both are clean now, so BOTH gate. The + split is kept because the two spellings need different error text: one + names a -bg token you can search for, the other names nothing at all. """ token_form, inline_form = [], [] for body in re.findall(r"\{([^{}]*)\}", css): - fg = set(re.findall(r"color\s*:\s*var\(\s*(--fs-[\w-]+?)\s*\)", body)) + # (? int: print("OK — no text painted with a token on a tint of its own -bg.\n") if inline_tint_hits: - by_tok: dict[str, int] = {} - for _p, tok in inline_tint_hits: - by_tok[tok] = by_tok.get(tok, 0) + 1 - print(f"REPORT — {len(inline_tint_hits)} rule(s) paint text with a token on " - f"an INLINE color-mix tint of that same token.") - print(" Same defect, spelled without a -bg token so it does not gate yet.") - print(" Worst offenders: " + ", ".join( - f"{t} x{n}" for t, n in sorted(by_tok.items(), key=lambda kv: -kv[1])[:4]) + "\n") + print(f"FAIL — {len(inline_tint_hits)} rule(s) paint text with a token on an " + f"INLINE color-mix tint of that same token.") + print(" Identical defect to the block above, spelled without a -bg token —") + print(" which is what let it hide: nothing about it LOOKS like the pattern.") + print(" Use the token's -fg sibling.\n") + for path, tok in inline_tint_hits: + print(f" {path}: color: var({tok}) on an inline tint -> var({tok}-fg)") + print() + else: + print("OK — no text painted with a token on an inline tint of itself.\n") if args.report_literals: print(f"REPORT — {literal_count} raw colour literal(s) in component CSS.") print(" Advisory: a literal is a value stated outside the system, so it " "cannot follow a palette change.\n") - return 1 if (unresolved or same_hue_hits) else 0 + return 1 if (unresolved or same_hue_hits or inline_tint_hits) else 0 if __name__ == "__main__": diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 93bc5e0..ad2e0a2 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -30,6 +30,21 @@ from quart import Quart # duplicate gate, the untagged-record systems_hint) act in-band in tool # responses, at the moment they apply. # Grow one of those, not this block. +# BUDGET: ~1980 of the client's ~2048-char cap (#2562). Everything below is +# competing for the last ~68 characters, so an addition here is a trade, never +# an append. +# +# Milestone 317 (a note's own verify_with / expires_when, and the sweep over +# them) was DECLINED a line, deliberately, by the operator — not overlooked. +# The reasoning, so it is not re-litigated blind: this is a map, and its own +# closing line says each tool's description carries the full contract. The +# sweep is a curation act, not a session-start reflex like enter_project or +# list_always_on_rules. Spending the last of the budget on it would leave the +# map unable to grow for something more central later. +# +# The accepted cost: an agent that never opens create_note's docstring never +# learns the field exists. Guidance lives in the create_note / update_note +# docstrings and the using-scribe skill instead. _INSTRUCTIONS = """ Scribe is the operator's self-hosted second brain and system of record — and yours: recall from it before acting, record as you go. Keep no parallel copy @@ -120,6 +135,13 @@ _READ_ONLY_TOOLS = frozenset({ # prefix, so the completeness test below cannot derive it — the same # reason `enter_project` is spelled out above. "retrieval_telemetry", + # The note staleness sweep (milestone 317). A pure read — mark_note_verified + # is the write, and it is deliberately NOT here. Spelled out for + # retrieval_telemetry's reason: `notes_due_for_verification` matches none of + # the prefixes the completeness test derives from, so nothing would have + # prompted this decision. `rules_due_for_verification` is in the same + # position and is NOT listed — see #3191. + "notes_due_for_verification", }) # Read-SHAPED tools that must NOT be reachable with a read key — a getter that diff --git a/src/scribe/mcp/tools/milestones.py b/src/scribe/mcp/tools/milestones.py index 5faf873..39b144d 100644 --- a/src/scribe/mcp/tools/milestones.py +++ b/src/scribe/mcp/tools/milestones.py @@ -137,11 +137,23 @@ async def delete_milestone(milestone_id: int) -> dict: """Move a milestone to the trash (recoverable). Its tasks go with it as one batch. Restore via restore(batch_id).""" uid = current_user_id() + # Read the title BEFORE the delete: afterwards the row is trashed and the + # confirmation could only echo the number back. A deletion the operator + # cannot recognise is one they cannot tell was the wrong one. + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + doomed = await milestones_svc.get_milestone(uid, milestone_id) + title = getattr(doomed, "title", "") if doomed else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "milestone", milestone_id) if batch is None: raise ValueError(f"milestone {milestone_id} not found") - return {"deleted_batch_id": batch, - "message": f"Milestone {milestone_id} + its tasks moved to trash. Restore with restore('{batch}')."} + return {"deleted": milestone_id, "title": title, "deleted_batch_id": batch, + "message": f'Milestone {milestone_id} ("{title}") and its tasks ' + f"moved to trash. Restore with restore('{batch}')."} def register(mcp) -> None: diff --git a/src/scribe/mcp/tools/notes.py b/src/scribe/mcp/tools/notes.py index d8b3931..aa5195c 100644 --- a/src/scribe/mcp/tools/notes.py +++ b/src/scribe/mcp/tools/notes.py @@ -103,6 +103,8 @@ async def create_note( project_id: int = 0, system_ids: list[int] | None = None, supersedes: list[int] | None = None, + verify_with: str = "", + expires_when: str = "", force: bool = False, ) -> dict: """Create a new note in Scribe. @@ -134,6 +136,33 @@ async def create_note( arrives labelled when it does surface. This records a CLAIM, not a verdict: it never says the older note was wrong, only that it is no longer the current answer. + verify_with: HOW TO CHECK this note is still true. Leave empty for + almost every note — that is the normal case, not an unfinished + one. + A note is a NORM or a CONSTRAINT. A norm is a decision ("we derive + versions from commit time"): it has no truth value and changes only + when its author changes it, which they know they did. A CONSTRAINT + asserts a fact about someone else's software ("AMO refuses to + re-sign a version", "this forge numbers CI runs per repository"), + and it goes false with nobody watching. Only constraints get a + check. + The test, in one question: COULD THIS NOTE BECOME FALSE WITHOUT + ANYONE EDITING IT? If no, leave this empty. Sharper still: is the + thing this note describes YOURS TO CHANGE? If yes it is a + decision. Measured against a real corpus, every note that earned a + check was about somebody else's software. + Three that look like candidates and are not: a resume pointer or + "current state" note (goes stale fastest, but the cure is to + update it, not to check it); a measurement of your own system (it + goes false because you changed something, and you knew); and a + decision that RESTS on someone else's behaviour (check the note + asserting the fact, not the decision). + A command, a path, a URL, a query. Prose is allowed; something + runnable is better. + expires_when: The STATE that ends it — deliberately not a date. + "When Forgejo issues run numbers per workflow rather than per + repository", not "in six months". Constraints expire when the + ground moves, not on a schedule. force: Bypass the near-duplicate gate. By default, if a title- or meaning-similar note already exists in the same project, creation is BLOCKED and the existing note's id is returned so you update it @@ -161,6 +190,8 @@ async def create_note( body=body, tags=tags, project_id=project_id or None, + verify_with=verify_with, + expires_when=expires_when, ) if system_ids: await systems_svc.set_record_systems(uid, note.id, system_ids) @@ -185,6 +216,9 @@ async def update_note( project_id: int = 0, system_ids: list[int] | None = None, supersedes: list[int] | None = None, + verify_with: str = "", + expires_when: str = "", + clear: list[str] | None = None, ) -> dict: """Update an existing Scribe note. Only explicitly provided fields are changed. @@ -199,6 +233,27 @@ async def update_note( supersedes: Replace the ids of earlier notes this one replaces (set-semantics). None = leave unchanged; [] = clear all. See create_note for when to reach for it. + verify_with: How to check this note is still true. Almost every note + should leave this empty — that is the normal, finished state, not + a gap: an empty check is the marker for "this is a decision, there + is nothing to go and check". See create_note for the full + norm-vs-constraint test; the short form is "could this become + false without anyone editing it?". + expires_when: The STATE that ends it, not a date. + clear: Names of fields to UNSET — "verify_with", "expires_when". + Needed because "" means "leave this alone" here, so there is no + value that removes a field: an agent updating a body must not + silently wipe a check it was not asked about. A note that stops + being a constraint is cleared by naming the field, which cannot + happen by accident. + + Rewriting `verify_with` drops the note's verification stamp: a stamp + certifies a particular check, and carrying it across a rewrite would vouch + for something nobody has looked at. + + A task and a snippet are both REFUSED a check, with a message saying where + to go instead — a task's decay is its status, and a snippet has + verify_snippet. """ uid = current_user_id() fields: dict = {} @@ -210,7 +265,13 @@ async def update_note( fields["tags"] = tags if project_id: fields["project_id"] = project_id - note = await notes_svc.update_note(uid, note_id, **fields) + if verify_with: + fields["verify_with"] = verify_with + if expires_when: + fields["expires_when"] = expires_when + note = await notes_svc.update_note( + uid, note_id, clear=clear or (), **fields + ) if note is None: raise ValueError(f"note {note_id} not found") if system_ids is not None: @@ -263,11 +324,113 @@ async def find_duplicate_records(kind: str = "note", threshold: float = 0.0) -> async def delete_note(note_id: int) -> dict: """Move a Scribe note to the trash (recoverable). Restore via restore(batch_id).""" uid = current_user_id() + # Read the title BEFORE the delete: afterwards the row is trashed and the + # confirmation could only echo the number back. A deletion the operator + # cannot recognise is one they cannot tell was the wrong one. + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + loaded = await notes_svc.get_note_for_user(uid, note_id) + title = getattr(loaded[0], "title", "") if loaded else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "note", note_id) if batch is None: raise ValueError(f"note {note_id} not found") - return {"deleted_batch_id": batch, - "message": f"Note {note_id} moved to trash. Restore with restore('{batch}')."} + return {"deleted": note_id, "title": title, "deleted_batch_id": batch, + "message": f'Note {note_id} ("{title}") moved to trash. ' + f"Restore with restore('{batch}')."} + + +async def notes_due_for_verification( + older_than_days: int = 0, project_id: int = 0, never_only: bool = False, +) -> dict: + """Which notes assert a FACT that nobody has confirmed lately. + + A corpus of notes holds two kinds of thing. Most are DECISIONS or records + of what happened — they have no truth value and cannot rot. A few assert a + fact about someone else's software: what a signing service does on a + duplicate upload, how a forge numbers its CI runs, what an updater + compares. Those go false silently, with nobody present, and a + cross-project reference note keeps being read as current by every project + that cites it. + + This lists the second kind, oldest verification first, NEVER-CHECKED AT + THE TOP — a note nobody has ever confirmed is a claim with no evidence + behind it at all. Each row carries `verify_with` in full, because you are + about to go and run it, plus `expires_when` and `days_since_verified`. + + Reach for it when curating, when a note's claim just contradicted what you + observed, or periodically. Then, per row: run the check, and call + mark_note_verified with what you found. + + Notes with no `verify_with` never appear, and that is correct — they are + decisions, and there is nothing to go and check. Do not "fix" their + absence by giving them checks: this list is only worth reading while + everything on it genuinely can go false. + + Args: + older_than_days: only notes last verified longer ago than this. + Never-checked notes always qualify — they are the most overdue + thing there is. 0 = no age filter. + project_id: narrow to one project. 0 = every project. Unlike the rules + sweep, this filter is safe: a note belongs to at most one project + outright, with none of the subscription and always-on paths that + would make a project filter UNDER-report a rule. + never_only: only notes nobody has ever verified. + """ + uid = current_user_id() + notes = await notes_svc.notes_due_for_verification( + uid, + older_than_days=older_than_days, + project_id=project_id or None, + never_only=never_only, + ) + return { + "notes": [notes_svc.verification_row(n) for n in notes], + "total": len(notes), + } + + +async def mark_note_verified(note_id: int, still_true: bool = True) -> dict: + """Record that you ran a note's check — and what it said. + + Call this AFTER actually running the note's `verify_with`, never on the + strength of the claim sounding plausible. A stamp nobody earned is worse + than no stamp: it moves the note to the bottom of the sweep and buys the + claim another long silence. + + `still_true=False` writes NOTHING. A note whose check failed is not in a + special state to be recorded — it is WRONG, and the honest next moves are + to correct it, supersede it, or find out why. So it stays at the top of + the sweep until someone deals with it, and the response tells you what the + note said would end it. + + Args: + note_id: the note whose check you ran. + still_true: True if the check passed. False if the fact it asserts is + no longer true — say so, that is the outcome worth having. + """ + uid = current_user_id() + note = await notes_svc.mark_note_verified(note_id, uid, still_true) + if note is None: + raise ValueError( + f"note {note_id} not found, not writable by you, or carries no " + f"verify_with (nothing to verify is not the same as verified)" + ) + data = notes_svc.verification_row(note) + data["verified"] = bool(still_true) + if not still_true: + data["next"] = ( + "This note is no longer true and is still being read as current " + "by anything that cites it. Correct it with update_note, write " + "the replacement with create_note(supersedes=[...]), or clear its " + "check with update_note(clear=[\"verify_with\"]) if it has stopped " + "asserting a fact at all. It stays at the top of " + "notes_due_for_verification until one of those happens." + ) + return data def register(mcp) -> None: @@ -278,5 +441,7 @@ def register(mcp) -> None: update_note, find_duplicate_records, delete_note, + notes_due_for_verification, + mark_note_verified, ): mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index a5c2dde..ca6a8c3 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -122,8 +122,9 @@ async def delete_rulebook(rulebook_id: int, confirmed: bool = False) -> dict: "confirmed_required": True, } batch = await trash_svc.delete(uid, "rulebook", rulebook_id) - return {"deleted": rulebook_id, "deleted_batch_id": batch, - "message": f"Moved to trash. Restore with restore('{batch}')."} + return {"deleted": rulebook_id, "title": rb.title, "deleted_batch_id": batch, + "message": f'Rulebook {rulebook_id} ("{rb.title}") moved to trash. ' + f"Restore with restore('{batch}')."} # ── Topic CRUD ───────────────────────────────────────────────────────── @@ -192,8 +193,9 @@ async def delete_topic(topic_id: int, confirmed: bool = False) -> dict: "confirmed_required": True, } batch = await trash_svc.delete(uid, "topic", topic_id) - return {"deleted": topic_id, "deleted_batch_id": batch, - "message": f"Moved to trash. Restore with restore('{batch}')."} + return {"deleted": topic_id, "title": topic.title, "deleted_batch_id": batch, + "message": f'Topic {topic_id} ("{topic.title}") moved to trash. ' + f"Restore with restore('{batch}')."} # ── Rule CRUD ────────────────────────────────────────────────────────── @@ -262,7 +264,16 @@ async def list_always_on_rules(project_id: int = 0) -> dict: """ uid = current_user_id() rules = await rulebooks_svc.list_always_on_rules(uid, project_id=project_id) - return {"rules": [_rule_summary(r) for r in rules], "total": len(rules)} + return { + "rules": [_rule_summary(r) for r in rules], + "total": len(rules), + # A marker for the set you are now holding. It is not for you to read: + # the write-path hook carries it back and is told if these rules have + # moved since. Deliberately NOT on rules_payload's applicable_rules — + # that is a DIFFERENT set (subscription-derived), and one key name + # over two sets is how a comparison starts reporting phantom changes. + "rules_etag": rulebooks_svc.rules_etag(rules), + } async def get_rule(rule_id: int) -> dict: @@ -548,6 +559,73 @@ async def update_rule( return await rulebooks_svc.rule_detail(uid, rule, system_ids) +async def rule_history(rule_id: int, version_id: int = 0) -> dict: + """What a rule USED TO SAY, newest change first. + + Read this before you argue with a rule, and before you rewrite one. A + rule that has been reworded may have been reworded for a reason you are + about to rediscover the hard way — and the wording it replaced is often + the fastest way to see what the current one is guarding against. The + rescoping of rule 79 is the case this exists for: the superseded + statement had to be hand-copied into a task log to survive the edit. + + EACH ENTRY HOLDS THE TEXT THE EDIT REPLACED, not the text it introduced. + So "what did this say before the most recent change?" is the first entry, + and the text the change PRODUCED is the rule as it stands now — read that + with get_rule. Pair the two and you have the diff. + + An empty history is ordinary and means the rule has never been reworded, + not that its history was lost. Nothing is written before milestone 323, + so a rule edited before then starts empty too. + + Args: + rule_id: The rule whose history to read. + version_id: 0 (default) lists the history — when each change + happened, by whom, and the title as it then stood. Pass an id + from that list to read that snapshot IN FULL. The list omits + statement and why on purpose: a rule's statement runs to + thousands of characters, and a history carrying every field would + cost more to read than the answer is worth. + + There is deliberately no restore. Putting an old wording back is a + decision, so it goes through update_rule — which snapshots what it + replaces, leaving the undo visible in the history like any other edit. A + one-click revert would erase the only record of why the rewrite happened. + """ + uid = current_user_id() + if version_id: + version = await rulebooks_svc.get_rule_version(rule_id, version_id, uid) + if version is None: + raise ValueError( + f"version {version_id} not found on rule {rule_id}" + ) + return version.to_dict(include_text=True) + + versions = await rulebooks_svc.list_rule_versions(rule_id, uid) + if versions is None: + raise ValueError(f"rule {rule_id} not found") + # Fail-open, like the deletes: a missing title must not turn a readable + # history into an error. + try: + rule = await rulebooks_svc.get_rule(rule_id, uid) + except Exception: + rule = None + return { + "rule_id": rule_id, + "title": rule.title if rule else "", + "versions": [v.to_dict(include_text=False) for v in versions], + "total": len(versions), + # Said in-band because an empty list is the ordinary case and reads + # like a missing feature otherwise. + "note": ( + "Each entry holds the text the edit REPLACED. The current wording " + "is on the rule itself — get_rule(%d)." % rule_id + if versions else + "This rule has never been reworded." + ), + } + + async def delete_rule(rule_id: int, confirmed: bool = False) -> dict: """Move a rule to the trash (recoverable). Requires confirmed=True.""" uid = current_user_id() @@ -563,8 +641,9 @@ async def delete_rule(rule_id: int, confirmed: bool = False) -> dict: "confirmed_required": True, } batch = await trash_svc.delete(uid, "rule", rule_id) - return {"deleted": rule_id, "deleted_batch_id": batch, - "message": f"Moved to trash. Restore with restore('{batch}')."} + return {"deleted": rule_id, "title": rule.title, "deleted_batch_id": batch, + "message": f'Rule {rule_id} ("{rule.title}") moved to trash. ' + f"Restore with restore('{batch}')."} # ── Subscriptions ────────────────────────────────────────────────────── @@ -832,5 +911,6 @@ def register(mcp) -> None: suppress_topic_for_project, unsuppress_topic_for_project, exclude_always_on_rulebook, include_always_on_rulebook, rules_due_for_verification, mark_rule_verified, + rule_history, ): mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/mcp/tools/snippets.py b/src/scribe/mcp/tools/snippets.py index faf7d5b..5b5c563 100644 --- a/src/scribe/mcp/tools/snippets.py +++ b/src/scribe/mcp/tools/snippets.py @@ -494,9 +494,19 @@ async def delete_snippet(snippet_id: int) -> dict: that should survive, prefer merge_snippets — that keeps the call sites. """ uid = current_user_id() + # Read before deleting so the confirmation can NAME what went — an id + # alone leaves the operator unable to tell which snippet this was. + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + doomed = await snippets_svc.get_snippet(uid, snippet_id) + title = getattr(doomed, "title", "") if doomed else "" + except Exception: + title = "" if not await snippets_svc.delete_snippet(uid, snippet_id): raise ValueError(f"snippet {snippet_id} not found") - return {"deleted": True, "id": snippet_id} + return {"deleted": True, "id": snippet_id, "title": title} async def merge_snippets(target_id: int, source_ids: list[int]) -> dict: diff --git a/src/scribe/mcp/tools/tasks.py b/src/scribe/mcp/tools/tasks.py index 3a17aef..e097292 100644 --- a/src/scribe/mcp/tools/tasks.py +++ b/src/scribe/mcp/tools/tasks.py @@ -364,11 +364,23 @@ async def delete_task(task_id: int) -> dict: """Move a Scribe task (or plan) to the trash (recoverable). Sub-tasks go with it. Restore via restore(batch_id).""" uid = current_user_id() + # Read the title BEFORE the delete: afterwards the row is trashed and the + # confirmation could only echo the number back. A deletion the operator + # cannot recognise is one they cannot tell was the wrong one. + # Fail-open: the title is a COURTESY on top of the delete, so a lookup + # that errors must not stop the delete happening. Same posture the + # staleness marker takes — a decoration may never break its payload. + try: + loaded = await notes_svc.get_note_for_user(uid, task_id) + title = getattr(loaded[0], "title", "") if loaded else "" + except Exception: + title = "" batch = await trash_svc.delete(uid, "task", task_id) if batch is None: raise ValueError(f"task {task_id} not found") - return {"deleted_batch_id": batch, - "message": f"Task {task_id} moved to trash. Restore with restore('{batch}')."} + return {"deleted": task_id, "title": title, "deleted_batch_id": batch, + "message": f'Task {task_id} ("{title}") moved to trash. ' + f"Restore with restore('{batch}')."} def register(mcp) -> None: diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index d55a116..203d962 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -33,6 +33,7 @@ from scribe.models.milestone import Milestone # noqa: E402, F401 from scribe.models.task_log import TaskLog # noqa: E402, F401 from scribe.models.note_draft import NoteDraft # noqa: E402, F401 from scribe.models.note_version import NoteVersion # noqa: E402, F401 +from scribe.models.rule_version import RuleVersion # noqa: E402, F401 from scribe.models.note_supersession import NoteSupersession # noqa: E402, F401 from scribe.models.group import Group, GroupMembership # noqa: E402, F401 from scribe.models.share import NoteShare, ProjectShare # noqa: E402, F401 diff --git a/src/scribe/models/embedding.py b/src/scribe/models/embedding.py index e206334..2cd510c 100644 --- a/src/scribe/models/embedding.py +++ b/src/scribe/models/embedding.py @@ -51,7 +51,11 @@ class RuleEmbedding(Base): """One embedding vector per CHUNK of a rule (milestone 307, note 3026). A SIBLING of NoteEmbedding rather than a generalisation of it, decided - deliberately: + deliberately. The reasoning below turned out to be the only written + statement of a rule Scribe applies to every record type, so it is now also + NOTE 3163 — "When a record type earns its own table" — with the worked + cases and the cost of leaving `notes`. Read that before splitting a record + type off; this stays here because it is where the decision was made. - The embedding ROW could have been made polymorphic. The SEARCH could not. `semantic_search_notes` is a long function of Note-specific scoping — diff --git a/src/scribe/models/note.py b/src/scribe/models/note.py index 86b995c..f9d51b3 100644 --- a/src/scribe/models/note.py +++ b/src/scribe/models/note.py @@ -94,9 +94,40 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): # name/language/signature/locations live here so they can be INDEXED. The # body keeps the same facts in readable markdown and remains what gets # embedded; this is a mirror for querying, not the source of truth for - # display. NULL on every row written before migration 0070, so readers fall - # back to parsing the body (see services/snippets.snippet_fields). + # display — and it is DERIVED, so every path that writes a snippet's body + # rewrites it too (services/snippets.recompose_data, called from + # notes.update_note). 0070 left it NULL on existing rows and + # snippets.backfill_snippet_data filled them at startup; readers still fall + # back to parsing the body when it is absent (snippet_fields). data: Mapped[dict | None] = mapped_column(JSONB, nullable=True) + # The three fields that tell a CONSTRAINT apart from a NORM (milestone + # 317, migration 0092) — the same trio `rules` carries, and for the same + # reason. A norm is a decision: no truth value, changes only when its + # author changes it. A constraint asserts a fact about someone else's + # software and goes false with nobody watching. Only constraints get a + # check. + # + # `verify_with` is how to check it is still true; `expires_when` is the + # STATE that ends it, deliberately not a date — constraints expire when + # the ground moves, not on a schedule. `verified_at` NULL means never + # checked and sorts FIRST in the sweep: unexamined outranks + # examined-long-ago. + # + # These sit on `notes`, so every kind of row in this table has them, but + # only non-task, non-snippet records are OFFERED them (gated in + # services/notes.py). A task's decay is its status — a done issue records + # what happened and cannot go false — and a snippet already carries a + # richer, location-aware verdict in `data.verification`. On those rows + # these stay null, which is also what they mean. + # + # Most notes should leave all three empty. A null `verify_with` is not a + # gap; it is the marker for "this is a decision, there is nothing to go + # and check", and the sweep is only worth reading while that holds. + verify_with: Mapped[str | None] = mapped_column(Text, nullable=True) + expires_when: Mapped[str | None] = mapped_column(Text, nullable=True) + verified_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) __table_args__ = ( Index("ix_notes_tags", "tags", postgresql_using="gin"), @@ -137,6 +168,14 @@ class Note(Base, TimestampMixin, SoftDeleteMixin): "is_task": self.is_task, "note_type": self.note_type or "note", "task_kind": self.task_kind, + # Serialized unconditionally, like every other field a given row + # kind may not use (recurrence, started_at, the task fields). The + # DERIVED "last_verified" label is the one that appears only when + # a check exists — a raw projection of the row should not make a + # client branch on which keys are present. + "verify_with": self.verify_with or "", + "expires_when": self.expires_when or "", + "verified_at": iso(self.verified_at), "created_at": iso(self.created_at), "updated_at": iso(self.updated_at), } diff --git a/src/scribe/models/rule_version.py b/src/scribe/models/rule_version.py new file mode 100644 index 0000000..a8f8fac --- /dev/null +++ b/src/scribe/models/rule_version.py @@ -0,0 +1,91 @@ +from datetime import datetime + +from sqlalchemy import BigInteger, ForeignKey, Text +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base +from scribe.models.base import CreatedAtMixin, iso + + +class RuleVersion(Base, CreatedAtMixin): + """One snapshot of a rule's text, taken before an edit overwrote it. + + THE SIBLING NOTES ALREADY HAD. `note_versions` has existed for a long + time, and the design-system note calls its history "the changelog". Rules + — which BIND BEHAVIOUR on every session that loads them — had nothing, so + an edit destroyed what the rule used to say. Rescoping rule 79 on + 2026-08-29 meant hand-copying the superseded statement into a task log to + keep it (#3237). The more consequential record had the weaker protection. + + `CreatedAtMixin`, not `TimestampMixin`: a version is an EVENT. It is + written once and never updated, so an `updated_at` on it would be a column + that can only ever lie. + + WHAT IS DELIBERATELY DIFFERENT FROM NoteVersion (milestone 323): + + - `user_id` is the ACTOR — who made the edit — where NoteVersion's is the + owner, because `update_note` passes an owner-scoped id. For an audit + trail over a binding instruction, "who changed this" is the question + being asked, and a rule is editable by anyone with rulebook access. + - No `pin_kind` / `pin_label`. Those exist so a note's version can survive + autosave pruning. Nothing prunes here, so a pin would protect a row that + was never at risk. + - TEXT ONLY. A rule's Systems and its typed relations are edges with their + own lifecycle; folding them in would make one word, "version", mean two + different things — the rule's wording, and the rule's place in the + graph. `verified_at` is likewise absent: it is a stamp about a check, + not a property of the text, and step 2's snapshot is taken before + update_rule clears it precisely so the history shows the check that was + in force when this wording was written. + """ + + __tablename__ = "rule_versions" + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + rule_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("rules.id", ondelete="CASCADE"), index=True + ) + # The actor. SET NULL rather than CASCADE: deleting a user must not erase + # the history of the rules they edited — the edit still happened, and the + # rule is still binding because of it. NoteVersion cascades because a + # note's versions belong to its owner; a rule's belong to the rule. + user_id: Mapped[int | None] = mapped_column( + BigInteger, ForeignKey("users.id", ondelete="SET NULL"), nullable=True + ) + + title: Mapped[str] = mapped_column(Text, default="") + statement: Mapped[str] = mapped_column(Text, default="") + why: Mapped[str | None] = mapped_column(Text, nullable=True) + how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) + when_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) + tier: Mapped[str | None] = mapped_column(Text, nullable=True) + verify_with: Mapped[str | None] = mapped_column(Text, nullable=True) + expires_when: Mapped[str | None] = mapped_column(Text, nullable=True) + + def to_dict(self, include_text: bool = True) -> dict: + """The row. `include_text=False` gives the listing form. + + A rule's `statement` and `why` run to thousands of characters — rule + 149's `why` alone is longer than most notes — so a history LIST that + carried every field would be unreadable and expensive. The listing + answers "when, and by whom"; opening one answers "and what did it + say". Same split NoteVersion makes with `include_body`. + """ + out: dict = { + "id": self.id, + "rule_id": self.rule_id, + "user_id": self.user_id, + "title": self.title, + "created_at": iso(self.created_at), + } + if include_text: + out.update({ + "statement": self.statement, + "why": self.why or "", + "how_to_apply": self.how_to_apply or "", + "when_to_apply": self.when_to_apply or "", + "tier": self.tier or "", + "verify_with": self.verify_with or "", + "expires_when": self.expires_when or "", + }) + return out diff --git a/src/scribe/routes/knowledge.py b/src/scribe/routes/knowledge.py index 3127c1a..b30ce93 100644 --- a/src/scribe/routes/knowledge.py +++ b/src/scribe/routes/knowledge.py @@ -1,4 +1,4 @@ -"""Unified Knowledge endpoint — notes, tasks, plans, and processes in one queryable feed.""" +"""Unified Knowledge endpoint — every record kind in one queryable feed.""" import logging from quart import Blueprint, jsonify, request @@ -6,12 +6,18 @@ from quart import Blueprint, jsonify, request from scribe.auth import get_current_user_id, login_required from scribe.routes.utils import parse_pagination from scribe.services.access import label_shared_items +from scribe.services.knowledge import FACET_TYPES logger = logging.getLogger(__name__) knowledge_bp = Blueprint("knowledge", __name__, url_prefix="/api/knowledge") -_VALID_TYPES = {"note", "task", "plan", "process"} +# Derived from the service's facet table, never re-listed here. This set was a +# hand-kept copy and had drifted three kinds behind it: it admitted `plan` +# (retired in 0066) and rejected `issue` (shipped in 0065, 435 rows) and +# `snippet` — so the browse surface could not filter to the kinds it was +# already rendering badges for (#3128). +_VALID_TYPES = FACET_TYPES _VALID_SORTS = {"modified", "created", "alpha", "type"} @@ -21,7 +27,9 @@ async def list_knowledge(): """Return paginated knowledge objects with optional filtering. Query params: - type — one of note|task|plan|process (omit for all) + type — a facet from services.knowledge._FACETS: a record type + (note|process|snippet) or a task kind (task for any, + else work|issue|spike|plan). Omit for all. tags — comma-separated tag filter (AND logic) sort — modified|created|alpha|type (default: modified) q — search query (semantic when provided, keyword fallback) @@ -127,7 +135,7 @@ async def get_knowledge_batch(): @knowledge_bp.route("/tags", methods=["GET"]) @login_required async def list_knowledge_tags(): - """Return all tags used across knowledge objects (excludes tasks).""" + """Return all tags used across knowledge objects, narrowed to one facet.""" uid = get_current_user_id() note_type = request.args.get("type", "").strip().lower() or None diff --git a/src/scribe/routes/notes.py b/src/scribe/routes/notes.py index 17a13ad..58b3779 100644 --- a/src/scribe/routes/notes.py +++ b/src/scribe/routes/notes.py @@ -19,7 +19,10 @@ from scribe.services.notes import ( get_note_for_user, get_or_create_note_by_title, list_notes, + mark_note_verified, + notes_due_for_verification, update_note, + verification_row, ) from scribe.services.note_drafts import upsert_draft, get_draft, delete_draft from scribe.services import dedup as dedup_svc @@ -112,6 +115,8 @@ async def create_note_route(): priority=priority, due_date=due_date, note_type=note_type, + verify_with=data.get("verify_with"), + expires_when=data.get("expires_when"), ) except ValueError as e: return jsonify({"error": str(e)}), 400 @@ -248,7 +253,14 @@ async def update_note_route(note_id: int): owner_uid = note_obj.user_id data = await request.get_json() fields = {} - for key in ("title", "body", "description", "parent_id", "project_id", "milestone_id", "status", "priority", "note_type"): + for key in ( + "title", "body", "description", "parent_id", "project_id", + "milestone_id", "status", "priority", "note_type", + # A cleared form input arrives as "" and the service reads that as + # NULL (NULLABLE_NOTE_TEXT), so this door expresses "remove the check" + # with its own idiom and needs no `clear` list (milestone 317). + "verify_with", "expires_when", + ): if key in data: fields[key] = data[key] if "due_date" in data: @@ -490,3 +502,62 @@ async def graph_route(): shared_tags = request.args.get("shared_tags", "false").lower() == "true" graph = await build_note_graph(uid, project_id=project_id, include_shared_tags=shared_tags) return jsonify(graph) + + +# ── The staleness sweep (milestone 317) ────────────────────────────────────── +# The web half of notes_due_for_verification / mark_note_verified. Same +# contract as the MCP door and the rules routes beside it — the service holds +# the behaviour, these two just parse and serialise. + +@notes_bp.route("/due-for-verification", methods=["GET"]) +@login_required +async def notes_due_route(): + """Notes that carry a check, oldest verification first, never-checked top. + + Query params: older_than_days, project_id, never_only. A note with no + `verify_with` never appears — it is a decision, not a fact. + """ + uid = get_current_user_id() + args = request.args + try: + older = int(args.get("older_than_days", 0) or 0) + project = int(args.get("project_id", 0) or 0) + except ValueError: + return jsonify({"error": "older_than_days and project_id must be integers"}), 400 + try: + notes = await notes_due_for_verification( + uid, + older_than_days=older, + project_id=project or None, + never_only=args.get("never_only", "").lower() in ("1", "true", "yes"), + ) + except ValueError as exc: + # A 400, not a silently narrowed result: a filter that quietly answers + # a different question is the failure this whole surface exists to + # catch. + return jsonify({"error": str(exc)}), 400 + return jsonify({ + "notes": [verification_row(n) for n in notes], + "total": len(notes), + }) + + +@notes_bp.route("//verify", methods=["POST"]) +@login_required +async def mark_note_verified_route(note_id: int): + """Record that the note's check was run. Body: {"still_true": bool}. + + `still_true: false` writes nothing — a note whose check failed is wrong, + not in a recordable state — so it keeps its place at the top of the sweep. + """ + data = await request.get_json() or {} + uid = get_current_user_id() + still_true = bool(data.get("still_true", True)) + note = await mark_note_verified(note_id, uid, still_true) + if note is None: + return jsonify({ + "error": "note not found, not writable by you, or carries no verify_with" + }), 404 + payload = verification_row(note) + payload["verified"] = still_true + return jsonify(payload) diff --git a/src/scribe/routes/plugin.py b/src/scribe/routes/plugin.py index 14e9d9b..b951413 100644 --- a/src/scribe/routes/plugin.py +++ b/src/scribe/routes/plugin.py @@ -138,6 +138,11 @@ async def write_path_prior_art(): or `canon:`) already named this session by the ledger arm (#2900); its own channel, like the two above. + rules_etag (opt) — the marker the session was given when it loaded + its always-on rules (milestone 323). Sent back + so the server can say whether those rules have + MOVED since. Absent means the hook has nothing + stored, which is silence, not a mismatch. shapes (opt) — comma-separated `kind:name` definitions the hook found in (or enclosing) the payload, kind being css|sym. The shape ledger's write-path feed @@ -157,6 +162,7 @@ async def write_path_prior_art(): p.strip() for p in (request.args.get("exclude_derive") or "").split(",") if p.strip() ] exclude_rule_ids = _int_list(request.args.get("exclude_rule_ids")) + rules_etag = (request.args.get("rules_etag") or "").strip() shapes = _parse_shapes(request.args.get("shapes") or "") api_key = getattr(g, "api_key", None) may_stamp = api_key is None or getattr(api_key, "scope", "") == "write" @@ -168,6 +174,7 @@ async def write_path_prior_art(): repo_key=repo_bindings_svc.normalize_repo_key(repo) if repo else "", exclude_derive=exclude_derive, exclude_rule_ids=exclude_rule_ids, + rules_etag=rules_etag, ) return jsonify(result) diff --git a/src/scribe/routes/rulebooks.py b/src/scribe/routes/rulebooks.py index 2acc4bb..629ba1c 100644 --- a/src/scribe/routes/rulebooks.py +++ b/src/scribe/routes/rulebooks.py @@ -206,6 +206,42 @@ async def update_rule(rule_id: int): return jsonify(await rulebooks_svc.rule_detail(uid, rule, data.get("system_ids"))) +@rulebooks_bp.get("/rules//versions") +@login_required +async def list_rule_versions(rule_id: int): + """A rule's edit history, newest first. + + Listing form only — a rule's `statement` and `why` run to thousands of + characters, so a history list carrying every field would be unreadable + and expensive to send. Open one for the text. + """ + uid = get_current_user_id() + versions = await rulebooks_svc.list_rule_versions(rule_id, uid) + if versions is None: + return jsonify({"error": "rule not found"}), 404 + return jsonify({ + "versions": [v.to_dict(include_text=False) for v in versions], + }) + + +@rulebooks_bp.get("/rules//versions/") +@login_required +async def get_rule_version(rule_id: int, version_id: int): + """One snapshot in full — what the rule said before that edit.""" + uid = get_current_user_id() + version = await rulebooks_svc.get_rule_version(rule_id, version_id, uid) + if version is None: + return jsonify({"error": "version not found"}), 404 + return jsonify(version.to_dict(include_text=True)) + + +# NO restore route, deliberately (milestone 323). A note version can be +# restored; a binding instruction should not be revertible in one click. +# Putting a rewrite back goes through update_rule, which takes its own +# snapshot and leaves the undo in the history like any other edit — a silent +# revert would erase the only record of why the rewrite happened. + + @rulebooks_bp.post("/rules//relations") @login_required async def relate_rules(rule_id: int): diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index dd23826..a791824 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -9,6 +9,7 @@ from scribe.models.note import Note from scribe.models.note_draft import NoteDraft from scribe.models.note_supersession import NoteSupersession from scribe.models.note_version import NoteVersion +from scribe.models.rule_version import RuleVersion from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.note_usage import NoteUsageEvent from scribe.models.canonical_system import CanonicalSystem @@ -50,8 +51,19 @@ logger = logging.getLogger(__name__) # ones (reference/hook) travel too, cheaply, and the next refresh refreshes them. # v10 (2026-08) added projects.inception + project_rulebook_exclusions # (milestone 297): the WHY a project inherits what it does, and its opt-outs. +# v11 (2026-08) added the note verification trio — notes.verify_with / +# expires_when / verified_at (milestone 317). +# v12 (2026-08) closed #3182: the nine Note columns that had been missing for +# years (note_type, task_kind, arose_from_id, data, description, the +# recurrence pair, started_at, completed_at), plus milestones.body — which IS +# the plan — and repo_bindings.ref. Until v12 a restore reported success and +# handed back a corpus with every snippet and process flattened into a plain +# note, every issue and spike into `work`, and every plan reduced to a title. +# _COLUMN_EXCLUSIONS and its guard landed with it, so the next such column +# fails the build instead. +# v13 (2026-08) added rule_versions — a rule's edit history (milestone 323). # Bump when the serialized schema changes. -BACKUP_VERSION = 10 +BACKUP_VERSION = 13 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -77,6 +89,9 @@ _BACKED_UP = [ "canonical_systems", # v10 (2026-08): a rule's area tag and its typed edges (milestone 307). "rule_systems", "rule_relations", + # v13 (2026-08): a rule's edit history (milestone 323). note_versions has + # always travelled; its sibling has no excuse not to. + "rule_versions", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -108,6 +123,86 @@ _NOT_INCLUDED = [ ] +# Columns a backed-up table deliberately does NOT export, per table. Paired +# with the column-coverage guard in tests/test_services_backup.py, this is +# _NOT_INCLUDED's shape one level down — and it exists because the table guard +# could not see the failure it was written to stop. +# +# #2293 was six whole tables missing. #3182 was NINE COLUMNS missing from a +# table that had been "covered" for years: note_type and task_kind, so every +# snippet and process restored as a plain note and every issue and spike as +# `work`; arose_from_id, so every provenance edge vanished; the recurrence +# pair, so recurring tasks stopped recurring; milestones.body, which IS the +# plan; repo_bindings.ref, the branch a ledger follows. Every one arrived the +# same way — a column added to the model and the migration, both of which fail +# loudly, and then never added to the serialiser, which fails silently. +# +# So: a new column on a backed-up table now fails the build unless it is either +# exported or named here with a reason. "I forgot" is no longer expressible. +_COLUMN_EXCLUSIONS: dict[str, set[str]] = { + # Trash is not exported at all, so neither is the batch id that groups a + # deletion for restore(). Uniform across every soft-deletable table. + "users": set(), + "projects": { + "deleted_at", "deleted_batch_id", + # Credentials-adjacent, same reasoning as api_keys and + # forge_connections: a restored project falls back to keyring-by-host + # resolution, which is the documented unpinned behaviour (#2778). + "forge_connection_id", + }, + "milestones": {"deleted_at", "deleted_batch_id"}, + "notes": {"deleted_at", "deleted_batch_id"}, + "task_logs": set(), + "note_drafts": set(), + "note_versions": set(), + "settings": set(), + "rulebooks": {"deleted_at", "deleted_batch_id"}, + "rulebook_topics": {"deleted_at", "deleted_batch_id"}, + "rules": {"deleted_at", "deleted_batch_id"}, + "systems": { + "deleted_at", "deleted_batch_id", + # Travels as `canonical_slug`: the catalog is global and its ids are + # per-install, so an id would restore pointing at whatever area + # happened to land on that number. + "canonical_id", + "created_at", "updated_at", + }, + "canonical_systems": { + "deleted_at", "deleted_batch_id", + # Matched on SLUG at restore, so a target install that already seeded + # the standard vocabulary reuses its own rows rather than colliding. + "id", "created_at", "updated_at", + }, + # Edge tables: the id is regenerated on insert, and the pair IS the row. + "record_systems": {"id", "created_at"}, + "note_supersessions": {"id", "created_at"}, + "rule_relations": {"id", "created_at"}, + "note_usage_events": {"id"}, + "design_systems": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, + "design_tokens": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, + "repo_bindings": {"id", "created_at", "updated_at"}, + # Everything travels. A version row IS the audit trail, so a column left + # behind is a fact about a binding instruction that no longer exists + # anywhere. + "rule_versions": set(), + # Serialised via the model's own to_dict(), so a column reaches the backup + # the moment it reaches that method — and the guard still catches one that + # reaches neither. + "code_shapes": { + # The PROPOSER's standing suggestion for an unclassified row, not a + # judgment: "looks like an instance of #N", or "repeats with no canon". + # Every refresh recomputes it and a judgment clears it, so carrying it + # would restore stale machine guesses over a tree the proposer has not + # seen. Same reasoning as code_shape_consumers in _NOT_INCLUDED — + # derived data is regenerated, never restored. + "proposed_snippet_id", "proposal_basis", "proposal_score", + "proposal_group", "proposed_at", "proposed_sha", + }, + "code_shape_events": set(), + "code_shape_uses": set(), +} + + def _dt(val: str | None) -> datetime: return datetime.fromisoformat(val) if val else datetime.now(timezone.utc) @@ -240,7 +335,13 @@ def _code_shape_use_rows(rows) -> list[dict]: def _repo_binding_rows(rows) -> list[dict]: return [ - {"user_id": r.user_id, "project_id": r.project_id, "repo_key": r.repo_key} + # `ref` is the branch this binding's ledger follows (#2873). Without + # it a restored binding silently falls back to the default branch, and + # the shape ledger starts accounting for a different tree (#3182). + { + "user_id": r.user_id, "project_id": r.project_id, + "repo_key": r.repo_key, "ref": r.ref, + } for r in rows ] @@ -282,6 +383,11 @@ def _milestone_rows(rows) -> list[dict]: { "id": m.id, "user_id": m.user_id, "project_id": m.project_id, "title": m.title, "description": m.description, "status": m.status, + # THE PLAN. A milestone IS the plan (0066) and `body` is its + # design and intent; `description` is only the one-line summary. + # Dropping this restored every plan as a title with no reasoning + # behind it (#3182). + "body": m.body, "order_index": m.order_index, "created_at": m.created_at.isoformat(), "updated_at": m.updated_at.isoformat(), @@ -294,12 +400,45 @@ def _note_rows(rows) -> list[dict]: return [ { "id": n.id, "user_id": n.user_id, "title": n.title, "body": n.body, + "description": n.description, "tags": n.tags or [], "parent_id": n.parent_id, "project_id": n.project_id, "milestone_id": n.milestone_id, "status": n.status, "priority": n.priority, "due_date": n.due_date.isoformat() if n.due_date else None, "created_at": n.created_at.isoformat(), "updated_at": n.updated_at.isoformat(), + # WHAT KIND OF RECORD THIS IS — both typing axes (#3182). Missing + # until now, which meant a restore reported success and handed back + # a corpus with every snippet and process flattened into a plain + # note and every issue and spike into `work`. Nothing recomputes + # these; the vocabulary is simply gone. + "note_type": n.note_type, + "task_kind": n.task_kind, + # Provenance — which record caused this one. Re-mapped in the + # second pass beside parent_id, never here: the value is an id in + # the SOURCE database. + "arose_from_id": n.arose_from_id, + # The queryable mirror. The only one of these that would self-heal + # (backfill_snippet_data rebuilds it from the body at startup), but + # a restore should not hand back a corpus that needs a restart to + # become searchable by location. + "data": n.data, + # Lifecycle: when the work actually started and finished, and the + # recurrence rule that makes a task come back. Without these a + # restored recurring task simply stops recurring. + "started_at": n.started_at.isoformat() if n.started_at else None, + "completed_at": n.completed_at.isoformat() if n.completed_at else None, + "recurrence_rule": n.recurrence_rule, + "recurrence_next_spawn_at": ( + n.recurrence_next_spawn_at.isoformat() + if n.recurrence_next_spawn_at else None + ), + # The verification trio (milestone 317, migration 0092). Operator + # judgment — "somebody checked this fact, and this is when" — + # which nothing can recompute. + "verify_with": n.verify_with, + "expires_when": n.expires_when, + "verified_at": n.verified_at.isoformat() if n.verified_at else None, } for n in rows ] @@ -342,6 +481,23 @@ def _note_version_rows(rows) -> list[dict]: ] +def _rule_version_rows(rows) -> list[dict]: + """A rule's edit history. Sibling of _note_version_rows, and it travels for + the same reason: a version is the only record of what a binding + instruction used to say, and nothing can recompute it.""" + return [ + { + "id": rv.id, "rule_id": rv.rule_id, "user_id": rv.user_id, + "title": rv.title, "statement": rv.statement, "why": rv.why, + "how_to_apply": rv.how_to_apply, "when_to_apply": rv.when_to_apply, + "tier": rv.tier, "verify_with": rv.verify_with, + "expires_when": rv.expires_when, + "created_at": rv.created_at.isoformat(), + } + for rv in rows + ] + + def _setting_rows(rows) -> list[dict]: return [{"user_id": s.user_id, "key": s.key, "value": s.value} for s in rows] @@ -423,6 +579,9 @@ async def export_full_backup() -> dict: note_versions = (await session.execute( select(NoteVersion).order_by(NoteVersion.note_id, NoteVersion.id) )).scalars().all() + rule_versions = (await session.execute( + select(RuleVersion).order_by(RuleVersion.rule_id, RuleVersion.id) + )).scalars().all() settings = (await session.execute(select(Setting))).scalars().all() systems = (await session.execute(select(System))).scalars().all() canonical_systems = (await session.execute( @@ -487,6 +646,7 @@ async def export_full_backup() -> dict: "task_logs": _task_log_rows(task_logs), "note_drafts": _note_draft_rows(note_drafts), "note_versions": _note_version_rows(note_versions), + "rule_versions": _rule_version_rows(rule_versions), "settings": _setting_rows(settings), "rulebooks": _rulebook_rows(rulebooks), "rulebook_topics": _topic_rows(topics), @@ -623,6 +783,14 @@ async def export_user_backup(user_id: int) -> dict: .join(CanonicalSystem, CanonicalSystem.id == rule_systems_t.c.canonical_id) .where(rule_systems_t.c.rule_id.in_(_rule_ids)) )).all() if _rule_ids else [] + # Scoped through the RULE, not the version's user_id. That column is + # the ACTOR (milestone 323), so filtering on it would carry the + # versions this user wrote on someone ELSE's rule and drop the ones + # someone else wrote on theirs — the opposite of a per-user export. + rule_versions = (await session.execute( + select(RuleVersion).where(RuleVersion.rule_id.in_(_rule_ids)) + .order_by(RuleVersion.rule_id, RuleVersion.id) + )).scalars().all() if _rule_ids else [] rule_relations = (await session.execute( select(RuleRelation).where( RuleRelation.from_rule_id.in_(_rule_ids), @@ -671,6 +839,7 @@ async def export_user_backup(user_id: int) -> dict: "task_logs": _task_log_rows(task_logs), "note_drafts": _note_draft_rows(note_drafts), "note_versions": _note_version_rows(note_versions), + "rule_versions": _rule_version_rows(rule_versions), "settings": _setting_rows(settings), "rulebooks": _rulebook_rows(rulebooks), "rulebook_topics": _topic_rows(topics), @@ -747,11 +916,29 @@ async def _restore_v1(data: dict) -> dict: body=n_data.get("body", ""), tags=n_data.get("tags", []), parent_id=None, # patched below + arose_from_id=None, # patched below, same reason + description=n_data.get("description"), + note_type=n_data.get("note_type") or "note", + task_kind=n_data.get("task_kind") or "work", + data=n_data.get("data"), + started_at=_dt_or_none(n_data.get("started_at")), + completed_at=_dt_or_none(n_data.get("completed_at")), + recurrence_rule=n_data.get("recurrence_rule"), + recurrence_next_spawn_at=_dt_or_none( + n_data.get("recurrence_next_spawn_at") + ), status=n_data.get("status"), priority=n_data.get("priority"), due_date=_d(n_data.get("due_date")), created_at=_dt(n_data.get("created_at")), updated_at=_dt(n_data.get("updated_at")), + verify_with=n_data.get("verify_with"), + expires_when=n_data.get("expires_when"), + # _dt_or_none, NOT _dt: an absent stamp must stay absent. _dt + # substitutes now(), which would restore every never-checked + # note as checked at the moment of the restore — inverting the + # one signal the sweep reads. + verified_at=_dt_or_none(n_data.get("verified_at")), ) session.add(note) await session.flush() @@ -759,14 +946,25 @@ async def _restore_v1(data: dict) -> dict: note_id_map[old_id] = note.id stats["notes"] += 1 - # Patch parent_id now that all notes have new IDs + # Patch the two note->note edges now that every note has a new id. + # Both are ids in the SOURCE database, so writing either straight into + # the constructor would point at whatever record happens to hold that + # number here — a restore that succeeds and silently re-parents (#3182). + # An edge whose target did not survive the import is left NULL rather + # than guessed at. for n_data in data.get("notes", []): old_id = n_data.get("id") + if not old_id or old_id not in note_id_map: + continue + note_row = await session.get(Note, note_id_map[old_id]) + if note_row is None: + continue old_parent = n_data.get("parent_id") - if old_id and old_parent and old_id in note_id_map and old_parent in note_id_map: - note_row = await session.get(Note, note_id_map[old_id]) - if note_row: - note_row.parent_id = note_id_map[old_parent] + if old_parent and old_parent in note_id_map: + note_row.parent_id = note_id_map[old_parent] + old_origin = n_data.get("arose_from_id") + if old_origin and old_origin in note_id_map: + note_row.arose_from_id = note_id_map[old_origin] for s_data in data.get("settings", []): mapped_user_id = user_id_map.get(s_data.get("user_id", 0)) @@ -799,7 +997,7 @@ async def _restore_v2(data: dict) -> dict: "design_tokens": 0, "note_usage_events": 0, "repo_bindings": 0, "note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0, "code_shape_uses": 0, "canonical_systems": 0, - "rule_systems": 0, "rule_relations": 0, + "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, } async with async_session() as session: @@ -859,6 +1057,7 @@ async def _restore_v2(data: dict) -> dict: project_id=mapped_pid, title=m_data.get("title", ""), description=m_data.get("description"), + body=m_data.get("body"), status=m_data.get("status", "active"), order_index=m_data.get("order_index", 0), created_at=_dt(m_data.get("created_at")), @@ -871,6 +1070,7 @@ async def _restore_v2(data: dict) -> dict: # 4a. Notes — first pass (no parent_id yet) notes_with_parents: list[tuple[int, int]] = [] # (new_note_id, old_parent_id) + notes_with_origins: list[tuple[int, int]] = [] # (new_note_id, old_arose_from_id) for n_data in data.get("notes", []): mapped_uid = user_id_map.get(n_data.get("user_id", 0)) if mapped_uid is None: @@ -881,6 +1081,17 @@ async def _restore_v2(data: dict) -> dict: body=n_data.get("body", ""), tags=n_data.get("tags", []), parent_id=None, + arose_from_id=None, # patched below, same reason as parent_id + description=n_data.get("description"), + note_type=n_data.get("note_type") or "note", + task_kind=n_data.get("task_kind") or "work", + data=n_data.get("data"), + started_at=_dt_or_none(n_data.get("started_at")), + completed_at=_dt_or_none(n_data.get("completed_at")), + recurrence_rule=n_data.get("recurrence_rule"), + recurrence_next_spawn_at=_dt_or_none( + n_data.get("recurrence_next_spawn_at") + ), project_id=project_id_map.get(n_data["project_id"]) if n_data.get("project_id") else None, milestone_id=milestone_id_map.get(n_data["milestone_id"]) if n_data.get("milestone_id") else None, status=n_data.get("status"), @@ -888,21 +1099,35 @@ async def _restore_v2(data: dict) -> dict: due_date=_d(n_data.get("due_date")), created_at=_dt(n_data.get("created_at")), updated_at=_dt(n_data.get("updated_at")), + verify_with=n_data.get("verify_with"), + expires_when=n_data.get("expires_when"), + # _dt_or_none — see the note on the other restore path. + verified_at=_dt_or_none(n_data.get("verified_at")), ) session.add(note) await session.flush() note_id_map[n_data["id"]] = note.id if n_data.get("parent_id"): notes_with_parents.append((note.id, n_data["parent_id"])) + if n_data.get("arose_from_id"): + notes_with_origins.append((note.id, n_data["arose_from_id"])) stats["notes"] += 1 - # 4b. Patch parent_id + # 4b. Patch the note->note edges. Deferred for the same reason + # parent_id always has been: these are ids in the SOURCE database + # (#3182). An edge whose target did not survive stays NULL. for new_note_id, old_parent_id in notes_with_parents: new_parent_id = note_id_map.get(old_parent_id) if new_parent_id: note_row = await session.get(Note, new_note_id) if note_row: note_row.parent_id = new_parent_id + for new_note_id, old_origin_id in notes_with_origins: + new_origin_id = note_id_map.get(old_origin_id) + if new_origin_id: + note_row = await session.get(Note, new_note_id) + if note_row: + note_row.arose_from_id = new_origin_id # 5. TaskLogs for tl_data in data.get("task_logs", []): @@ -1141,6 +1366,32 @@ async def _restore_v2(data: dict) -> dict: )) stats["rule_relations"] += 1 + # A rule's edit history (milestone 323). Must come after the rules + # themselves — rule_id_map is only populated above — and both ids are + # ids in the SOURCE database, which is #3182's arose_from_id trap. + for rv in data.get("rule_versions", []): + mapped_rid = rule_id_map.get(rv.get("rule_id", 0)) + if mapped_rid is None: + continue + # Unlike NoteVersion, an unmappable user does NOT drop the row. + # user_id is the ACTOR and is nullable by design: the column is + # SET NULL precisely so history outlives the account that wrote + # it. Skipping here would delete the record the FK preserves. + session.add(RuleVersion( + rule_id=mapped_rid, + user_id=user_id_map.get(rv.get("user_id") or 0), + title=rv.get("title", ""), + statement=rv.get("statement", ""), + why=rv.get("why"), + how_to_apply=rv.get("how_to_apply"), + when_to_apply=rv.get("when_to_apply"), + tier=rv.get("tier"), + verify_with=rv.get("verify_with"), + expires_when=rv.get("expires_when"), + created_at=_dt(rv.get("created_at")), + )) + stats["rule_versions"] += 1 + # 15. Systems for sy_data in data.get("systems", []): mapped_uid = user_id_map.get(sy_data.get("user_id", 0)) @@ -1255,6 +1506,7 @@ async def _restore_v2(data: dict) -> dict: session.add(RepoBinding( user_id=mapped_uid, project_id=mapped_pid, repo_key=rb_data.get("repo_key", ""), + ref=rb_data.get("ref"), )) stats["repo_bindings"] += 1 diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index b0c2229..a5506ad 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -1,4 +1,4 @@ -"""Knowledge service — unified query across notes, tasks, plans, and processes. +"""Knowledge service — one query across every record kind Scribe holds. ACL (rules #47/#78, decision note 2094): these queries were owner-only until 2026-07-25, which meant a record shared with you could be opened by id but never @@ -265,22 +265,94 @@ def _note_to_item(note: Note) -> dict: return item -def _apply_type_filter(stmt, note_type: str | None): - """Apply the type facet to a Note select. +# What each type facet MEANS, once, for every arm that has to know. +# +# The vocabulary spans BOTH typing axes — `note_type` for non-task records and +# `task_kind` for tasks — so a facet cannot be a filter on one column, which is +# why this is a table rather than a chain of ifs. Each entry is +# (is_task, the value pinned on that axis); None pins nothing, i.e. every task. +# +# It is a table because the alternative had already gone wrong. The predicate +# was written three times — a SQL if-chain, a Python if-chain over semantic +# candidates, and a ternary computing the `is_task` pre-filter — and the three +# only agreed by luck. Adding `issue` to the SQL arm alone (the obvious edit, +# and the one #3128 was about to make) would have set the pre-filter to +# is_task=False, handed the Python arm a candidate set containing no tasks at +# all, and returned an empty semantic half for the Issues facet forever, with +# nothing red anywhere. A new facet is now one row here. +# +# `plan` is retired (0066) but kept: 90 legacy plan-tasks exist and a facet +# they answer to costs one line. It simply has no chip in the UI any more. +_FACETS: dict[str, tuple[bool, str | None]] = { + "task": (True, None), + "work": (True, "work"), + "issue": (True, "issue"), + "spike": (True, "spike"), + "plan": (True, "plan"), + "note": (False, "note"), + "process": (False, "process"), + "snippet": (False, "snippet"), +} - 'task' = any task (status not null); 'plan' = a task with task_kind='plan'; - any other non-empty type = a non-task note of that note_type; None = all. +# The non-task record types, for the counts query. Derived so it cannot drift +# from the table above. +NON_TASK_FACETS = tuple( + value for _is_task, value in _FACETS.values() if not _is_task and value +) - Trashed rows (deleted_at set) are always excluded. +# The whole vocabulary, for the door's request validation — public so the route +# validates against the same table the query reads instead of a hand-kept copy. +FACET_TYPES = frozenset(_FACETS) + + +# An unrecognised facet resolves to "a non-task note whose note_type is that +# string" — which matches nothing, since no row stores an unknown type. That is +# the behaviour the old if-chain had by falling through, and it is the right +# one: a typo should return an empty list, never the whole corpus. +def _facet(note_type: str) -> tuple[bool, str | None]: + return _FACETS.get(note_type, (False, note_type)) + + +def facet_is_task(note_type: str | None) -> bool | None: + """The `is_task` pre-filter a facet implies — None when it spans both. + + Used to narrow the semantic candidate set before it is fetched. Reads the + same table `_apply_type_filter` and `matches_facet` read, so the pre-filter + can no longer disagree with the predicate it is meant to anticipate. """ + if not note_type: + return None + return _facet(note_type)[0] + + +def matches_facet(note, note_type: str | None) -> bool: + """The Python dialect of `_apply_type_filter`, for candidates the vector + search has already fetched — there is no query left to narrow. + + Generated from the same table, so this is a translation rather than a + second implementation. Note the `not note.is_task` arm: the hand-written + version omitted it and was saved only by the upstream pre-filter. + """ + if not note_type: + return True + is_task, value = _facet(note_type) + if is_task: + return note.is_task and (value is None or note.task_kind == value) + return not note.is_task and note.note_type == value + + +def _apply_type_filter(stmt, note_type: str | None): + """Apply the type facet to a Note select. Trashed rows are always excluded.""" stmt = stmt.where(Note.deleted_at.is_(None)) - if note_type == "task": - return stmt.where(Note.status.isnot(None)) - if note_type == "plan": - return stmt.where(Note.status.isnot(None)).where(Note.task_kind == "plan") - if note_type: - return stmt.where(Note.note_type == note_type).where(Note.status.is_(None)) - return stmt + if not note_type: + return stmt + is_task, value = _facet(note_type) + if is_task: + stmt = stmt.where(Note.status.isnot(None)) + if value is not None: + stmt = stmt.where(Note.task_kind == value) + return stmt + return stmt.where(Note.status.is_(None)).where(Note.note_type == value) async def query_knowledge( @@ -295,7 +367,7 @@ async def query_knowledge( locations: dict[str, str] | None = None, verification: str = "", ) -> tuple[list[dict], int]: - """Query knowledge objects (non-task notes) with filters. + """Query knowledge objects with filters. `project_id` narrows to one project (None = every project). @@ -424,7 +496,7 @@ async def _semantic_knowledge_search( INTERACTIVE_SEARCH_THRESHOLD, semantic_search_notes, ) - is_task_filter = True if note_type in ("task", "plan") else (False if note_type else None) + is_task_filter = facet_is_task(note_type) import time as _time _t0 = _time.perf_counter() candidates = await semantic_search_notes( @@ -454,11 +526,7 @@ async def _semantic_knowledge_search( for _score, note in candidates: if note.deleted_at is not None: continue - if note_type == "task" and not note.is_task: - continue - elif note_type == "plan" and (not note.is_task or note.task_kind != "plan"): - continue - elif note_type and note_type not in ("task", "plan") and note.note_type != note_type: + if not matches_facet(note, note_type): continue if tags and not all(t in (note.tags or []) for t in tags): continue @@ -514,51 +582,40 @@ async def get_knowledge_counts(user_id: int, tags: list[str] | None = None) -> d search would surface.""" visible = browsable_notes_clause(user_id) async with async_session() as session: - # Count non-task types - stmt = ( - select(Note.note_type, func.count(Note.id)) - .where(visible) - .where(Note.status.is_(None)) - .where(Note.deleted_at.is_(None)) - .where(Note.note_type.in_(["note", "process"])) - .group_by(Note.note_type) - ) - if tags: - for tag in tags: + def _scoped(stmt): + stmt = stmt.where(visible).where(Note.deleted_at.is_(None)) + for tag in tags or []: stmt = stmt.where(Note.tags.contains([tag])) - rows = list((await session.execute(stmt)).all()) - counts = {row[0]: row[1] for row in rows} + return stmt - # Count tasks separately (is_task = status IS NOT NULL) - task_stmt = ( - select(func.count(Note.id)) - .where(visible) + # One grouped query per typing axis. The task half used to be a count + # for 'task' plus a second count for 'plan', which is why 'issue' — + # 17% of every task here — had no number to show: each kind needed its + # own query and nobody added one. Grouping by task_kind counts every + # kind, including ones added later, for the same two round-trips. + non_task = _scoped( + select(Note.note_type, func.count(Note.id)) + .where(Note.status.is_(None)) + .where(Note.note_type.in_(NON_TASK_FACETS)) + ).group_by(Note.note_type) + counts = {t: n for t, n in (await session.execute(non_task)).all()} + + by_kind = _scoped( + select(Note.task_kind, func.count(Note.id)) .where(Note.status.isnot(None)) - .where(Note.deleted_at.is_(None)) - ) - if tags: - for tag in tags: - task_stmt = task_stmt.where(Note.tags.contains([tag])) - task_count: int = (await session.execute(task_stmt)).scalar_one() - counts["task"] = task_count + ).group_by(Note.task_kind) + kind_counts = {k: n for k, n in (await session.execute(by_kind)).all()} - # Plans are a subset of tasks (task_kind='plan'); counted for the facet - # but NOT added to total to avoid double-counting against "task". - plan_stmt = ( - select(func.count(Note.id)) - .where(visible) - .where(Note.status.isnot(None)) - .where(Note.task_kind == "plan") - .where(Note.deleted_at.is_(None)) - ) - if tags: - for tag in tags: - plan_stmt = plan_stmt.where(Note.tags.contains([tag])) - counts["plan"] = (await session.execute(plan_stmt)).scalar_one() + # Kinds are SUBSETS of 'task' and are deliberately left out of the total — + # adding them would count every task twice. + counts["task"] = sum(kind_counts.values()) + for kind, value in _FACETS.items(): + if value[0] and value[1] is not None: + counts[kind] = kind_counts.get(kind, 0) - for t in ("note", "task", "plan", "process"): + for t in NON_TASK_FACETS: counts.setdefault(t, 0) - counts["total"] = sum(counts[t] for t in ("note", "task", "process")) + counts["total"] = counts["task"] + sum(counts[t] for t in NON_TASK_FACETS) return counts diff --git a/src/scribe/services/notes.py b/src/scribe/services/notes.py index cf24f0f..db8d61d 100644 --- a/src/scribe/services/notes.py +++ b/src/scribe/services/notes.py @@ -1,5 +1,6 @@ import logging import re +from collections.abc import Iterable from datetime import date, datetime, timezone from sqlalchemy import func, or_, select, text @@ -9,6 +10,52 @@ from scribe.models.note import Note, TaskKind, TaskPriority, TaskStatus logger = logging.getLogger(__name__) +# The fields `snippets.parse_snippet_fields` reads. Writing any of them can +# change what a snippet's derived `data` mirror should say, so update_note +# recomposes the mirror when one moves. Kept here as a set of NAMES rather +# than imported, because it describes update_note's own `fields` dict, not the +# parser's signature. +_PARSED_FROM_BODY = frozenset({"title", "body", "tags"}) + + +# Text fields where EMPTY MEANS NULL (milestone 317). The sweep's whole signal +# is `verify_with IS NULL` = "this is a decision, there is nothing to go and +# check". An empty string that is not NULL makes a norm look like a constraint +# nobody has verified, forever — and it would sit at the top of the sweep, +# since never-checked sorts first. Sibling of rulebooks.NULLABLE_RULE_TEXT. +NULLABLE_NOTE_TEXT = ("verify_with", "expires_when") + + +def guard_check_fields(status: str | None, note_type: str | None) -> None: + """Raise unless a record in this shape may carry verify_with/expires_when. + + Stated as an INVARIANT over the resulting record rather than a filter on + which fields a caller passed, so it also catches the sideways route: a + checked note being turned into a task, which no per-field gate would see. + + Raises rather than dropping silently, for minted_kind's reason (#3129) — a + silently-corrected write is the defect that reasoning exists to end, and a + caller putting a check on the wrong record has an idea an error corrects + and a default hides. Lives at the service, not either door, so REST and MCP + cannot come to disagree about it. + """ + if status is not None: + raise ValueError( + "a task cannot carry verify_with/expires_when: a task's decay is " + "its status, and a done issue records what happened rather than " + "asserting something that can later go false. Put the check on the " + "note the fact lives in, or clear the check before making this a " + "task." + ) + if note_type == "snippet": + raise ValueError( + "a snippet already has a check: verify_snippet(), which compares " + "the recorded location and code against the repo and expires its " + "own verdict when the code moves. verify_with/expires_when are the " + "free-text form, for prose notes that assert a fact about " + "something outside the repo." + ) + def embed_note(note) -> None: """Refresh a note's embedding, fire-and-forget. @@ -101,7 +148,16 @@ async def create_note( task_kind: str = "work", arose_from_id: int | None = None, data: dict | None = None, + verify_with: str | None = None, + expires_when: str | None = None, ) -> Note: + # Empty means empty (NULLABLE_NOTE_TEXT), then the invariant. Both run + # before anything is written, so an illegal shape never reaches the table. + verify_with = verify_with or None + expires_when = expires_when or None + if verify_with or expires_when: + guard_check_fields(status, note_type) + # Validate status/priority here so the MCP create_task path (which passes # them straight through) can't persist an out-of-enum value that the REST # route would have rejected — there's no DB CHECK on notes.status. @@ -145,6 +201,8 @@ async def create_note( task_kind=task_kind, arose_from_id=arose_from_id, data=data, + verify_with=verify_with, + expires_when=expires_when, ) session.add(note) await session.commit() @@ -397,7 +455,20 @@ def minted_kind(kind: str) -> str: raise ValueError(f"kind must be one of {MINTABLE_KINDS}, got {kind!r}") -async def update_note(user_id: int, note_id: int, **fields: object) -> Note | None: +async def update_note( + user_id: int, note_id: int, clear: Iterable[str] = (), **fields: object, +) -> Note | None: + """Partial update. `clear` names fields to UNSET; **fields carries values. + + Clearing is explicit and separate because a nullable field cannot be + emptied by passing it: the MCP door reads "" as "leave this alone", so an + agent filling two fields does not wipe the others, and a note that stops + being a constraint genuinely needs its check removed. Naming the field is + the one form that cannot happen by accident. The REST door, where a + cleared form input arrives as "", reaches the same place through the + NULLABLE_NOTE_TEXT normalisation below — two idioms, one outcome. + (Same shape as rulebooks.update_rule, milestone 312 step 2.) + """ async with async_session() as session: result = await session.execute( select(Note).where(Note.id == note_id, Note.user_id == user_id) @@ -409,6 +480,10 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No old_body = note.body old_title = note.title old_tags = list(note.tags or []) + check_before = note.verify_with + for key in clear: + if key in NULLABLE_NOTE_TEXT: + setattr(note, key, None) for key, value in fields.items(): if not hasattr(note, key): continue @@ -438,7 +513,44 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No ) elif key == "tags" and isinstance(value, list): value = _normalize_tags(value) + elif key in NULLABLE_NOTE_TEXT: + value = value or None + elif key == "verified_at": + # Not settable here. A stamp says somebody performed THIS + # check, so it is written by the verification path and by a + # restore, never by an ordinary edit that could mint one for a + # check nobody ran. + continue setattr(note, key, value) + # The invariant, over the RESULTING record rather than over what was + # passed — which is what catches a checked note being turned into a + # task. Raised before commit, so nothing is persisted. + if note.verify_with or note.expires_when: + guard_check_fields(note.status, note.note_type) + # A stamp certifies A CHECK, not a record. Rewrite or remove the check + # and the old stamp certifies something that no longer exists, so it is + # dropped and the note re-enters the sweep. The safe direction: a note + # wrongly listed as due costs one look; a note wrongly vouched for + # costs exactly what the sweep exists to catch. + if note.verify_with != check_before: + note.verified_at = None + # A snippet's `data` is DERIVED from its body — so a write that moves + # the body through this generic door must move the mirror with it + # (#3128). Without this, PATCH /api/notes/ {body} left the + # mirror behind, and snippet_fields PREFERS the mirror: the row went on + # reporting its old repo/path/symbol to prior-art recall while showing + # its new body. `update_snippet` composes the mirror itself and passes + # it explicitly, so an explicit `data` always wins — the caller that + # knows the field set beats the one that can only re-read the body. + if "data" not in fields and not _PARSED_FROM_BODY.isdisjoint(fields): + # Imported here, not at module scope: services/snippets.py calls + # back into this module (update_snippet -> update_note), so a + # top-level import is a cycle. + from scribe.services.snippets import ( + SNIPPET_NOTE_TYPE, recompose_data, + ) + if note.note_type == SNIPPET_NOTE_TYPE: + note.data = recompose_data(note) # Auto-set lifecycle timestamps on status transitions if "status" in fields: _now = datetime.now(timezone.utc) @@ -487,6 +599,154 @@ async def update_note(user_id: int, note_id: int, **fields: object) -> Note | No # permanent deletion. Both are reachable; neither is spelled `delete_note`. +# ── The sweep (milestone 317 step 3) ───────────────────────────────────────── +# +# A SIBLING of rulebooks.rules_due_for_verification, not a shared +# implementation, and deliberately so (note 3163). The row could have been +# shared; the QUERY cannot. That sweep scopes by rulebook ownership XOR project +# ownership because rules have no sharing ACL at all — no rule_shares, no +# can_read_rule. A note scopes by the note ACL, which is a different question +# with a different answer. What IS common — how a stamp reads, how old it is — +# lives in services/verification.py and is imported by both. + +async def notes_due_for_verification( + user_id: int, + older_than_days: int = 0, + project_id: int | None = None, + never_only: bool = False, +) -> list[Note]: + """Notes that carry a check, oldest verification first, never-checked top. + + THE QUERY THE COLUMNS EXIST FOR. `verify_with` and `expires_when` are + storage; this is what turns them into something that gets acted on. + Without it, note decay is caught only when a human reads the note and + disagrees — which is the case where the note was already believed. + + Ordered `verified_at` ASC **NULLS FIRST**: never-checked outranks + checked-long-ago, because a note nobody has ever confirmed is a claim with + no evidence behind it at all. Postgres sorts NULLs LAST on ASC by default, + so this is explicit — and getting it wrong would not error, it would + silently invert the one signal the sweep exists to carry. + + Notes with no `verify_with` never appear. Not an omission: they are + decisions, there is nothing to go and check, and listing them would dilute + the result until nobody reads it. + + Scoped with `browsable_notes_clause`, NOT the read scope (decision note + 2094): a sweep is a passive surface, and a record shared one-to-one must + not arrive in one unasked. + + Deliberately NOT filtered to non-task, non-snippet records even though the + write path (step 2) permits a check on nothing else. A row in that state + would be a row in an ILLEGAL state, and this is the one surface that could + tell somebody about it. Hiding it here to match the invariant would make + the sweep agree with a database it had stopped describing. + + Args: + user_id: whose notes. + older_than_days: only notes last verified longer ago than this. + Never-checked notes always qualify — they are the most overdue + thing there is. 0 = no age filter. Negative raises: it would mean + "everything", which is a different question than the one asked, + answered silently. + project_id: narrow to one project. None = every project. + never_only: only notes that have never been verified. + """ + from datetime import timedelta + + from scribe.services.access import browsable_notes_clause + + if older_than_days < 0: + raise ValueError( + f"older_than_days must be >= 0, got {older_than_days}. A negative " + f"window silently means 'everything', which is not what any caller " + f"of a staleness sweep is asking." + ) + + async with async_session() as session: + # One statement, not a fetch-then-filter: the ordering below is the + # database's, so it cannot disagree with itself across two halves. + stmt = ( + select(Note) + .where( + browsable_notes_clause(user_id), + Note.deleted_at.is_(None), + Note.verify_with.is_not(None), + ) + ) + if project_id is not None: + stmt = stmt.where(Note.project_id == project_id) + if never_only: + stmt = stmt.where(Note.verified_at.is_(None)) + elif older_than_days > 0: + cutoff = datetime.now(timezone.utc) - timedelta(days=older_than_days) + stmt = stmt.where( + or_(Note.verified_at.is_(None), Note.verified_at < cutoff) + ) + stmt = stmt.order_by(Note.verified_at.asc().nullsfirst(), Note.id) + return list((await session.execute(stmt)).scalars().all()) + + +def verification_row(note: Note) -> dict: + """One row of the sweep — the CHECK in full, unlike a listing. + + The opposite call from a browse: here the caller is about to go and run the + check, so the text they need IS the payload rather than the bloat. + """ + from scribe.services.verification import ( + days_since_verified, + last_verified_label, + ) + + return { + "id": note.id, + "title": note.title, + "project_id": note.project_id, + "verify_with": note.verify_with or "", + "expires_when": note.expires_when or "", + "last_verified": last_verified_label(note), + "days_since_verified": days_since_verified(note), + } + + +async def mark_note_verified( + note_id: int, user_id: int, still_true: bool = True, +) -> Note | None: + """Stamp a note as verified — or, when the check FAILED, refuse to. + + The asymmetry is the design: passing writes a stamp, failing writes + nothing. There is no "verified false" state, because a note whose check + failed is not a note in a special condition — it is a note that is WRONG, + and the honest resolutions are to correct it, supersede it, or find out + why. Recording the failure as a flag would let it sit there being false + with the sweep quietly satisfied that somebody had looked. + + So a failed check leaves `verified_at` untouched and the note stays at the + top of the sweep until someone actually deals with it. + + Write access, not read (rules 47/78): stamping is a mutation, and an + editor-share holder may make it while a viewer may not. + + Returns None when the note is not found, not writable, or carries no + `verify_with` — nothing to verify is a different answer from verified. + """ + from scribe.services.access import can_write_note + + async with async_session() as session: + note = (await session.execute( + select(Note).where(Note.id == note_id, Note.deleted_at.is_(None)) + )).scalars().first() + if note is None or not note.verify_with: + return None + if not await can_write_note(user_id, note_id): + return None + if still_true: + note.verified_at = datetime.now(timezone.utc) + await session.commit() + await session.refresh(note) + return note + + async def get_all_tags(user_id: int, q: str | None = None) -> list[str]: async with async_session() as session: if q: diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 2759e84..f6a5066 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -708,6 +708,7 @@ async def build_write_path_hint( repo_key: str = "", exclude_derive: list[str] | None = None, exclude_rule_ids: list[int] | None = None, + rules_etag: str = "", ) -> dict: """Prior-art hint for the plugin's PreToolUse hook on Write/Edit. @@ -951,7 +952,74 @@ async def build_write_path_hint( derive = [d for d in found if d.get("key") not in skip] except Exception: logger.warning("write-time derive check failed", exc_info=True) - if not synced and not menu and not stamped and not divergence and not derive: + staleness: list[str] = [] + # ── Have the rules moved under this session? (milestone 323) ─────── + # + # THE CARRIER IS THE POINT. This hook already fires before a write — the + # moment acting on a stale rule actually costs something — and the check + # is one comparison against a marker the session already holds. No + # payload, no extra round trip, and nothing said when nothing moved. + # + # WHAT THIS CANNOT SEE, and a reader who finds an etag here will assume + # otherwise: + # + # what goes wrong | caught? + # ---------------------------------------------------|-------- + # another session edits a rule mid-flight | yes + # the session is misremembering a rule read hours ago | yes + # compaction summarised the rules out of context | NO + # + # The third is the most common and this is blind to it: the etag was in + # context too and went with the rules. The SessionStart nudge is that + # case's only mechanism and must not be softened because this shipped. + # + # Fails open, like every other arm here: a staleness hint must never + # break a write. + if rules_etag: + try: + current = await rulebooks_svc.list_always_on_rules( + user_id, project_id=project_id or 0, + ) + if rulebooks_svc.rules_etag(current) != rules_etag: + moved = rulebooks_svc.rules_moved_since(current, rules_etag) + held = rulebooks_svc.etag_count(rules_etag) + bits = [] + if moved: + named = ", ".join( + f"#{r.id} \u201c{r.title}\u201d" for r in moved[:3] + ) + more = len(moved) - 3 + bits.append( + f"{named}" + (f", and {more} more" if more > 0 else "") + ) + # A DELETED rule moves no timestamp and leaves no row to name, + # so the count is the only thing that can report the one change + # that takes an instruction OUT of force. + if held is not None and held != len(current): + delta = len(current) - held + bits.append( + f"{abs(delta)} rule(s) {'added' if delta > 0 else 'no longer in force'}" + ) + if bits: + staleness.append( + "Your loaded rules have changed since this session " + "started — " + "; ".join(bits) + ". Re-read them with " + "list_always_on_rules() before relying on the set you " + "are holding." + ) + except Exception: + logger.debug("write-path rules-etag arm failed", exc_info=True) + + # The guard sits BELOW the staleness arm on purpose. A rules change is + # unconditional news — it does not become less true because this + # particular write happened to match no prior art — and this arm is one + # indexed query, only when the session actually sent a marker. + # + # The standing-rule arm further down is deliberately left on the far side + # of this guard: that one runs a SEMANTIC search, and moving it here would + # run an embedding query on every write in the session. Its gating is a + # separate question from this one (see the note on #3244). + if not staleness and not synced and not menu and not stamped and not divergence and not derive: return empty owners = await owner_names_for({ @@ -970,7 +1038,9 @@ async def build_write_path_hint( for marker, item in menu: rendered.append((item, marker, _owner_of(item), _foreign_language(item, target_lang))) - lines: list[str] = [] + # Seeded with the staleness line, which is decided above the early + # return and so cannot wait for this list to exist. + lines: list[str] = list(staleness) sync_note_ids: list[int] = [] if synced: # The sync framing (#2708). Deliberately imperative about the record — @@ -1208,7 +1278,10 @@ async def build_session_context( its normalized key — triggers a one-line "bind this repo" hint so the binding is self-healing. - Returns {"context": str, "rule_count": int, "project": dict | None}. + Returns {"context": str, "rule_count": int, "project": dict | None, + "rules_etag": str}. The etag is for the HOOK, not for the model — the + hook stores it and hands it back on each write so the server can say + whether these rules have moved since the session loaded them. `context` is markdown ready to drop into `additionalContext`; it is capped at _MAX_CHARS with an explicit truncation note so the hook can pass it through verbatim. @@ -1317,4 +1390,12 @@ async def build_session_context( if len(context) > _MAX_CHARS: context = context[:_MAX_CHARS].rstrip() + "\n\n…(truncated — call list_always_on_rules())" - return {"context": context, "rule_count": len(rules), "project": project_dict} + return { + "context": context, + "rule_count": len(rules), + "project": project_dict, + # Computed from the rules THIS payload was built from, not re-queried: + # the marker has to describe the set the session is actually holding, + # and a second query could disagree with the first. + "rules_etag": rulebooks_svc.rules_etag(rules), + } diff --git a/src/scribe/services/rule_versions.py b/src/scribe/services/rule_versions.py new file mode 100644 index 0000000..2d905a4 --- /dev/null +++ b/src/scribe/services/rule_versions.py @@ -0,0 +1,82 @@ +"""A rule's edit history — the snapshot taken when an edit overwrites text. + +Sibling of `note_versions`, and the differences are the whole design. + +WHAT A VERSION IS FOR. It records what a rule USED TO SAY, because an update +overwrites that and nothing can recompute it. It is not a record of the rule's +lifecycle: a soft delete keeps the rule row whole and restorable, so there is +nothing to preserve at delete time, and anything that outlived a purge would be +data the operator explicitly asked to be gone. + +WHAT IS DELIBERATELY NOT COPIED FROM note_versions (milestone 323). Both of its +guards defend against note autosave, which fires every 60 seconds and would +otherwise burn every slot. Rules have no autosave — every version here comes +from a deliberate `update_rule` — so both guards would only ever discard a real +edit: + +- No `MIN_VERSION_INTERVAL_SECONDS`. Two deliberate edits four minutes apart + are two edits. +- No `MAX_VERSIONS` and no pruning. A rule is edited a handful of times in its + life; a cap invites losing the one edit somebody needed. + +TEXT ONLY. A rule's topic, its Systems tags and its typed edges are not here. +They have their own lifecycle, and folding them in would make one word, +"version", mean two things — the rule's wording, and the rule's place in the +graph. +""" +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.rule_version import RuleVersion + +# The columns a version carries, and therefore the ones whose change is worth +# a snapshot. `order_index` and `arose_from_id` are editable through +# update_rule and are deliberately absent: reordering a rulebook is not an +# edit to what any rule says, and a run of "moved rule 4 above rule 3" +# snapshots would bury the edits somebody is actually looking for. +SNAPSHOT_FIELDS = ( + "title", "statement", "why", "how_to_apply", "when_to_apply", + "tier", "verify_with", "expires_when", +) + + +def snapshot(rule) -> dict: + """The rule's text right now, as a plain dict. + + Plain values rather than the ORM object because the caller has to hold + this ACROSS the mutation — a live `rule` would show the new text by the + time it was read, which is the one thing the snapshot must not do. + """ + return {f: getattr(rule, f) for f in SNAPSHOT_FIELDS} + + +def record_if_changed(session, rule, actor_user_id: int | None, before: dict): + """Add a version holding `before`, unless the edit changed no text. + + Git semantics, and not a nicety: `update_rule` is reached by callers that + resend every field, so without this a form saved twice would file a second + identical snapshot and the history would stop reading as a list of edits. + + Session-BOUND, unlike `note_versions.create_version` which opens its own. + The version and the edit that caused it commit together or not at all; + a separate session could leave a snapshot behind after the update it + describes had failed, which is a history entry for an edit that never + happened. + """ + if snapshot(rule) == before: + return None + version = RuleVersion( + rule_id=rule.id, user_id=actor_user_id, **before, + ) + session.add(version) + return version + + +async def list_versions(rule_id: int) -> list[RuleVersion]: + """Newest first — the reader's question is "what did this just say?".""" + async with async_session() as session: + return list((await session.execute( + select(RuleVersion) + .where(RuleVersion.rule_id == rule_id) + .order_by(RuleVersion.created_at.desc(), RuleVersion.id.desc()) + )).scalars().all()) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 536754e..d2378c1 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -9,6 +9,7 @@ from __future__ import annotations import logging from collections.abc import Iterable +from datetime import datetime from typing import Optional from sqlalchemy import and_, delete as sql_delete, insert, or_, select @@ -16,6 +17,12 @@ from sqlalchemy import and_, delete as sql_delete, insert, or_, select from scribe.models import async_session from scribe.models.system import System from scribe.models.rulebook import Rulebook +from scribe.services.verification import ( + days_since_verified as _days_since_verified, + last_verified_label as _last_verified_label, +) +from scribe.services import rule_versions +from scribe.models.rule_version import RuleVersion logger = logging.getLogger(__name__) @@ -311,19 +318,12 @@ def _valid_tier(tier: str) -> str: return tier if tier in TIERS else "always_on" -def last_verified_label(rule: Rule) -> str | None: - """How long ago the rule's check passed — None when it carries no check. - - One helper because two surfaces need the same answer and the brief-dict - lesson in rule_brief's docstring is what happens otherwise: three copies - that had already drifted. `None` means "this rule is a decision, the - question does not apply"; "never" means "it is a fact and nobody has - confirmed it" — a distinction worth keeping, because the second is the - one worth acting on. - """ - if not rule.verify_with: - return None - return rule.verified_at.date().isoformat() if rule.verified_at else "never" +# Re-exported, not redefined. Notes gained the same trio in milestone 317 and +# this reading of it is genuinely common, so it moved to services/verification +# — the DRY win note 3163 names, as against sharing the QUERY, which the two +# record types cannot (a rule scopes by rulebook ownership, a note by the note +# ACL). Kept importable from here because callers already reach for it here. +last_verified_label = _last_verified_label def rule_brief(rule: Rule, **extra) -> dict: @@ -751,6 +751,10 @@ async def update_rule( "verify_with", "expires_when", } check_before = rule.verify_with + # Captured BEFORE anything is written, and as plain values — this has + # to survive the mutation below. A rule's history is the only record + # of what it used to say; the edit itself destroys that. + text_before = rule_versions.snapshot(rule) for key in clear: if key in allowed and key in NULLABLE_RULE_TEXT: setattr(rule, key, None) @@ -774,12 +778,59 @@ async def update_rule( # wrongly vouched for costs the thing the sweep exists to catch. if rule.verify_with != check_before: rule.verified_at = None + # Same session as the edit, so the two commit together. The snapshot + # holds the OLD verify_with — the check that was in force when that + # wording was written — which is why it is taken before the loop and + # not here. + rule_versions.record_if_changed(session, rule, user_id, text_before) await session.commit() await session.refresh(rule) _refresh_rule_embedding(rule) return rule +# ── Edit history (milestone 323) ─────────────────────────────────────── +# +# The ACL-scoped reads live HERE rather than in services/rule_versions.py, +# and not by preference: rulebooks imports rule_versions for the write path, +# so the reverse import would be a cycle. The split is also the honest one — +# rule_versions owns what a version IS, this module owns who may read one. + + +async def list_rule_versions(rule_id: int, user_id: int): + """A rule's history, newest first. None when the rule is not readable. + + Scoped through the rule itself, never through the version's `user_id`: + that column is the ACTOR. Reading a rule's history is a question about + the RULE, so anyone who can read the rule can read what it used to say, + and anyone who cannot read the rule gets nothing — including the versions + they personally wrote, if the rule has since moved out of their reach. + """ + async with async_session() as session: + if await _fetch_owned_rule(session, rule_id, user_id) is None: + return None + return await rule_versions.list_versions(rule_id) + + +async def get_rule_version(rule_id: int, version_id: int, user_id: int): + """One snapshot in full. None when the rule or the version is not found. + + Takes the rule id as well as the version id so the ownership check has + something to run against BEFORE the version is read, and so a version id + from another rule cannot be read through a rule the caller does happen to + own — the check and the fetch have to agree about which rule is in play. + """ + async with async_session() as session: + if await _fetch_owned_rule(session, rule_id, user_id) is None: + return None + return (await session.execute( + select(RuleVersion).where( + RuleVersion.id == version_id, + RuleVersion.rule_id == rule_id, + ) + )).scalar_one_or_none() + + # ── Canon tags + typed edges (milestone 307) ─────────────────────────── async def set_rule_systems( @@ -1363,6 +1414,101 @@ def rules_payload(applicable: dict) -> dict: } +# ── The staleness marker (milestone 323 step 5) ──────────────────────── +# +# WHAT THIS CAN AND CANNOT SEE. An etag catches a rule that MOVED after a +# session loaded it. It is not a general staleness check, and a reader who +# finds one here will assume it is: +# +# what goes wrong | caught? +# ---------------------------------------------------|-------- +# another session edits a rule mid-flight | yes +# the session is misremembering a rule read hours ago | yes +# compaction summarised the rules out of context | NO +# +# The third is the most common and the marker is blind to it, because the +# etag was in context too and went with the rules. The SessionStart nudge is +# that case's only mechanism, and MUST NOT be softened because this exists — +# retiring something that covers the common case in favour of something that +# does not is the plausible mistake here. + +_ETAG_EMPTY = "empty|0" + + +def rules_etag(rules: list) -> str: + """A marker for "is the set you are holding still the current one?". + + `max(updated_at)` alone is not enough: DELETING a rule moves no timestamp, + and that is the single change that takes an instruction OUT of force — + the one a session most needs to hear about. The count catches it. + + Instance-agnostic (rule 115): it knows nothing about any particular + rulebook, and an install with one rule or none produces a stable marker + rather than an error. "No rules" must read as a state, not as a change, + or every session on a fresh install would be told its rules had moved. + """ + if not rules: + return _ETAG_EMPTY + # A decoration must not be able to break what it decorates. This is + # computed on the SessionStart path, where raising would cost the whole + # context payload to save a hint — so a row with no usable timestamp is + # skipped rather than compared, and a set with none degrades to a + # count-only marker instead of failing. Count-only still catches a rule + # added or deleted; it just cannot see an edit, which is the right way + # round to lose information. + stamps = [ + r.updated_at for r in rules + if isinstance(getattr(r, "updated_at", None), datetime) + ] + if not stamps: + return f"unknown|{len(rules)}" + return f"{max(stamps).isoformat()}|{len(rules)}" + + +async def rules_etag_for(user_id: int, project_id: int = 0) -> str: + """The current marker for the set a session at this scope would hold. + + Deliberately built from `list_always_on_rules` rather than from a + `max()/count()` aggregate. An aggregate would be cheaper, and would have + to restate that function's definition of the set — the always_on flag, + the project's inception exclusions, the tier filter. Two definitions of + "the session's rules" is how the marker starts disagreeing with the + rules, which is worse than materialising a few dozen rows. + """ + rules = await list_always_on_rules(user_id, project_id=project_id) + return rules_etag(rules) + + +def rules_moved_since(rules: list, held_etag: str) -> list: + """The rules whose text changed after `held_etag` was issued. + + Returns [] when the marker matches, is unparseable, or is absent — a + caller cannot act on "something is different but I cannot say what", and + a garbled marker must not be reported as a change. + + A count difference is real news that this list cannot show: a rule + DELETED since the marker was issued has no row left to return. Callers + compare counts separately. + """ + if not held_etag or held_etag == _ETAG_EMPTY: + return [] + stamp, _, _count = held_etag.partition("|") + try: + held_at = datetime.fromisoformat(stamp) + except ValueError: + return [] + return [r for r in rules if r.updated_at and r.updated_at > held_at] + + +def etag_count(held_etag: str) -> int | None: + """How many rules the holder had. None when the marker cannot be read.""" + _stamp, _, count = (held_etag or "").partition("|") + try: + return int(count) + except ValueError: + return None + + # ── The staleness sweep (milestone 312) ──────────────────────────────── async def rules_due_for_verification( @@ -1455,14 +1601,7 @@ def verification_row(rule: Rule) -> dict: because "2026-06-14" and "74 days" prompt different reactions and only one of them is the question being asked. """ - from datetime import datetime, timezone - - days = None - if rule.verified_at is not None: - stamp = rule.verified_at - if stamp.tzinfo is None: - stamp = stamp.replace(tzinfo=timezone.utc) - days = (datetime.now(timezone.utc) - stamp).days + days = _days_since_verified(rule) return { "id": rule.id, "title": rule.title, diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 064b685..8e6fdb9 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -540,14 +540,60 @@ def compose_data( return out +def recompose_data(note) -> dict: + """Rebuild a snippet's `data` mirror from its own body, title and tags. + + For the GENERIC note door. `update_snippet` composes the mirror itself from + the field set it just merged and never needs this; a plain + `update_note(body=...)` — which the Knowledge feed's editor issues, because + a snippet card there routes to /notes/:id — has no idea the mirror exists, + and left it stale. `snippet_fields` then PREFERS the stale mirror, so the + row reported its old repo/path/symbol to prior-art recall while displaying + its new body: confidently wrong, which is worse than no record (#3128). + + The body is the authority; the mirror is derived. That is already the rule + this file states — it just had no enforcement on the path that bypasses + `update_snippet`. + + `verification` and `provenance` are CARRIED, not recomposed, because + neither is in the body to parse — the same carry `compose_data` does for + the snippet service's own writes. Note that a verdict does not need + invalidating here: `code_sha` is recomputed from the new code, so a stale + verdict expires itself on read exactly as it does after any other edit. + """ + parsed = parse_snippet_fields(note.title, note.body, note.tags) + prior = note.data or {} + return compose_data( + name=parsed["name"], + when_to_use=parsed["when_to_use"], + signature=parsed["signature"], + language=parsed["language"], + code=parsed["code"], + locations=parsed["locations"], + merged_from=parsed["merged_from"], + verification=prior.get("verification"), + provenance=prior.get("provenance"), + ) + + def snippet_fields(note) -> dict: """Structured fields for a snippet, preferring the indexed `data` column and falling back to parsing the body. - Both paths must agree, because rows written before migration 0070 have no - `data` and are never backfilled — a hand-edited body is the authority for - those, and there is no deadline by which they must be converted. `code` only - ever comes from the body, since `data` doesn't carry it. + THE BODY IS THE AUTHORITY; `data` is a mirror derived from it. Every writer + keeps them in step — the snippet service composes the mirror from the field + set it just merged, `update_note` recomposes it when a body reaches the + generic door (#3128), and `backfill_snippet_data` filled the pre-0070 rows + at startup. The fallback below is therefore a belt to that braces, not a + second source of truth: it is what a row looks like before the backfill has + run, and it must keep agreeing with the mirror. + + (This docstring used to say those rows were "never backfilled". That was + true when 0070 landed and stopped being true when the backfill shipped; it + is corrected here because the sentence read as licence for a stale mirror, + which is exactly the bug #3128 found.) + + `code` only ever comes from the body, since `data` doesn't carry it. """ parsed = parse_snippet_fields(note.title, note.body, note.tags) stored = getattr(note, "data", None) diff --git a/src/scribe/services/verification.py b/src/scribe/services/verification.py new file mode 100644 index 0000000..865e627 --- /dev/null +++ b/src/scribe/services/verification.py @@ -0,0 +1,57 @@ +"""What a record's own check MEANS — shared by rules and notes. + +Two record types carry `verify_with` / `expires_when` / `verified_at`: rules +(milestone 312) and notes (milestone 317). What they share is BEHAVIOUR — how +a stamp is read, how old it is, what "never" means — not storage and not the +query. Note 3163 is the rule this file is an instance of: `semantic_search_*` +could never have been shared between them because a rule scopes by rulebook +ownership and a note scopes by the note ACL, so the two sweeps are siblings. +These functions are the part that genuinely is common, factored so it exists +once rather than twice. + +Everything here is pure, sync and DUCK-TYPED: it reads `verify_with` and +`verified_at` off whatever it is handed. That is deliberate. A shared base +class or a protocol would tie two SQLAlchemy models together to share four +lines of date arithmetic — the DRY costume, in the same note's words. +""" +from datetime import datetime, timezone + + +def last_verified_label(record) -> str | None: + """How long ago this record's check passed — None when it carries none. + + Three states, and the distinction between the last two is the whole point: + + - `None` — this record is a DECISION. There is nothing to go and check, + and the question does not apply. Most records. + - "never" — it asserts a fact and NOBODY HAS EVER CONFIRMED IT. The one + worth acting on, and why the sweep sorts these first. + - a date — somebody checked, then. + + Callers attach this to a payload only when it is not None, so "no key" and + "never verified" do not become two states every client has to tell apart. + """ + if not getattr(record, "verify_with", None): + return None + stamp = getattr(record, "verified_at", None) + return stamp.date().isoformat() if stamp else "never" + + +def days_since_verified(record) -> int | None: + """Whole days since the check last passed; None if it never has. + + Computed rather than left to the reader, because "2026-06-14" and "74 + days" prompt different reactions and only one of them is the question + being asked. + + Naive stamps are read as UTC. Postgres hands these back tz-aware, but a + restore, a fixture or a sqlite-backed test may not, and subtracting an + aware datetime from a naive one raises — which would make the sweep fail + on exactly the rows it exists to surface. + """ + stamp = getattr(record, "verified_at", None) + if stamp is None: + return None + if stamp.tzinfo is None: + stamp = stamp.replace(tzinfo=timezone.utc) + return (datetime.now(timezone.utc) - stamp).days diff --git a/tests/helpers.py b/tests/helpers.py index df77475..45ac4a0 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -9,7 +9,64 @@ from __future__ import annotations from contextlib import contextmanager from datetime import datetime, timezone from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock, MagicMock, patch + + +async def drive_update_note(note, **kwargs): + """Run `services/notes.update_note` against a stand-in row. + + The patch stack is the point: update_note reaches for a version snapshot, + an embedding refresh and a project reactivation on its way out, none of + which a unit test has. Written twice — once for the snippet mirror + (#3128) and once for the verification fields (#3182/317) — before being + consolidated here. + + Returns whatever update_note returned; assert on the `note` you passed in. + """ + from unittest.mock import AsyncMock as _AsyncMock + + session = make_mock_session() + result = MagicMock() + result.scalars.return_value.first.return_value = note + session.execute = _AsyncMock(return_value=result) + with patch("scribe.services.notes.async_session") as cls, \ + patch("scribe.services.notes.embed_note", MagicMock()), \ + patch("scribe.services.notes._maybe_reactivate_project", _AsyncMock()), \ + patch("scribe.services.note_versions.create_version", _AsyncMock()): + cls.return_value = session + from scribe.services.notes import update_note + return await update_note(user_id=7, note_id=note.id, **kwargs) + + +def tool_doc(module: str, name: str) -> str: + """An MCP tool's docstring, whitespace-flattened. + + Flattened because these are hard-wrapped at ~76 characters, so any phrase + worth asserting on is liable to straddle a line break — a property of the + formatter, not of the guidance. The disambiguator guard (#3123) learned + that on its own first run, matching raw text and reporting a phrase absent + that was plainly there. + + Used by every test that pins the docstring CONTRACT rather than its + wording. The tool docstring is the agent-facing contract (rule 119), so + these guards exist to catch it being tidied down to a parameter list. + """ + import importlib + import re as _re + + fn = getattr(importlib.import_module(module), name) + assert fn.__doc__, f"{name} has no docstring at all" + return _re.sub(r"\s+", " ", fn.__doc__) + + +def compiled_sql(element) -> str: + """A SQLAlchemy clause or statement rendered as literal SQL text. + + For asserting on the shape of a predicate without a database — which is how + the visibility clauses and the knowledge facets are both tested. Was a + private copy in each of those modules before #3128 needed a third. + """ + return str(element.compile(compile_kwargs={"literal_binds": True})) def make_mock_session() -> AsyncMock: @@ -92,6 +149,9 @@ def fake_note(**attrs) -> MagicMock: "id": 1, "title": "t", "body": "", "tags": [], "user_id": 7, "note_type": "note", "is_task": False, "task_kind": "work", "data": None, "deleted_at": None, + # Milestone 317: a truthy mock here reads as "this note carries a + # check", which trips the guard on records that may not have one. + "verify_with": None, "expires_when": None, "verified_at": None, }, attrs) @@ -101,6 +161,7 @@ def fake_task(**attrs) -> MagicMock: "id": 1, "title": "t", "body": "", "status": "todo", "priority": "none", "tags": [], "parent_id": None, "project_id": None, "is_task": True, "task_kind": "work", "user_id": 7, "deleted_at": None, + "verify_with": None, "expires_when": None, "verified_at": None, }, attrs) @@ -112,6 +173,8 @@ def fake_snippet(**attrs) -> MagicMock: "body": "```js\nreturn 1\n```\n", "tags": ["js", "snippet"], "note_type": "snippet", "is_task": False, "task_kind": "work", "user_id": 7, "data": None, "deleted_at": None, + "status": None, + "verify_with": None, "expires_when": None, "verified_at": None, }, attrs) diff --git a/tests/test_create_tools_disambiguate.py b/tests/test_create_tools_disambiguate.py index 360009c..0089707 100644 --- a/tests/test_create_tools_disambiguate.py +++ b/tests/test_create_tools_disambiguate.py @@ -29,6 +29,7 @@ parameters, with the "is this even the right tool" paragraph quietly gone. import re import pytest +from tests.helpers import tool_doc as _doc # The create surfaces and where they live. `start_planning` is here because # it is a create in everything but name — it is how a plan comes into being. @@ -50,21 +51,6 @@ _ALTERNATIVES = [ ] -def _doc(module: str, name: str) -> str: - """The docstring with its whitespace flattened. - - Flattened because a docstring is hard-wrapped: "design system" spans a - line break in at least one of these, and matching the raw text would - report it absent. The first draft of this check did exactly that, and - caught it on itself. - """ - import importlib - - fn = getattr(importlib.import_module(module), name) - assert fn.__doc__, f"{name} has no docstring at all" - return re.sub(r"\s+", " ", fn.__doc__) - - @pytest.mark.parametrize(("module", "name"), _SURFACES) def test_a_create_surface_names_at_least_two_alternatives(module, name): """Reaching for the wrong tool must still put the right one in view.""" diff --git a/tests/test_integration_backup_note_roundtrip.py b/tests/test_integration_backup_note_roundtrip.py new file mode 100644 index 0000000..f6210fc --- /dev/null +++ b/tests/test_integration_backup_note_roundtrip.py @@ -0,0 +1,207 @@ +"""Real-Postgres round trip for the note fields #3182 restored. + +The unit lane can prove a serialiser EMITS a key. It cannot prove a restore +puts the value back on the right row, and the note->note edges are exactly +where that distinction bites: `parent_id` and `arose_from_id` hold ids from +the SOURCE database, so a restore that writes them straight through succeeds, +reports success, and silently points every edge at whatever record happens to +hold that number here. + +That is why #3182 was not fixed in passing. + +These drive the REAL `restore_full_backup`, not a reimplementation of its +loop. A test that re-derives the remap it is checking would agree with +whatever the product does, including nothing. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.note import Note +from scribe.models.user import User +from scribe.services import backup +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +RESTORED_USERNAME = "backup_roundtrip_restored" + + +@pytest_asyncio.fixture +async def source(): + """A snippet, a process, and an issue that arose from a work task — the + shapes whose identity the backup used to drop — exported as backup rows. + + Deleted children-first: `arose_from_id` is a real FK, so removing the + origin while the issue still points at it is asking the database a + question the test has no reason to ask. + """ + async with async_session() as s: + owner = await ensure_user(s, "backup_roundtrip_owner") + uid = owner.id + await s.commit() + + async with async_session() as s: + origin = Note( + user_id=uid, title="the work that broke", body="", + status="done", task_kind="work", + ) + snippet = Note( + user_id=uid, title="debounce — rate-limit", body="```js\n1\n```", + note_type="snippet", data={"name": "debounce", "language": "js"}, + ) + process = Note( + user_id=uid, title="DRY pass", body="steps", note_type="process", + ) + s.add_all([origin, snippet, process]) + await s.commit() + origin_id = origin.id + + issue = Note( + user_id=uid, title="the fix", body="symptom -> cause -> fix", + status="done", task_kind="issue", arose_from_id=origin_id, + description="one-liner", + ) + s.add(issue) + await s.commit() + order = [issue.id, origin_id, snippet.id, process.id] + + async with async_session() as s: + rows = (await s.execute(select(Note).where(Note.id.in_(order)))).scalars().all() + note_rows = backup._note_rows(list(rows)) + user_rows = backup._user_rows( + [(await s.execute(select(User).where(User.id == uid))).scalars().one()] + ) + # Restore mints a NEW user from the payload, so the restored corpus is + # entirely separate from the source — which is what makes the id + # assertions meaningful. Renamed here rather than in a sibling fixture: + # `restored` depends on this one, and a rename elsewhere might not have + # run by the time the restore does. + user_rows[0]["username"] = RESTORED_USERNAME + + yield { + "payload": { + "version": backup.BACKUP_VERSION, + "users": user_rows, + "notes": note_rows, + }, + "origin_id": origin_id, + "owner_id": uid, + } + + async with async_session() as s: + for nid in order: + row = await s.get(Note, nid) + if row is not None: + await s.delete(row) + await s.commit() + + +@pytest_asyncio.fixture +async def restored(source): + """Run the real restore, then hand back the new rows by title. + + The restore mints a NEW user from the payload, so the restored corpus is + entirely separate from the source one — which is what makes the id + assertions below meaningful. + """ + await backup.restore_full_backup(source["payload"]) + + async with async_session() as s: + user = (await s.execute( + select(User).where(User.username == RESTORED_USERNAME) + )).scalars().first() + assert user is not None, "the payload's user was not restored" + rows = (await s.execute( + select(Note).where(Note.user_id == user.id) + )).scalars().all() + by_title = {n.title: n for n in rows} + new_user_id = user.id + + yield by_title, source + + async with async_session() as s: + fresh = [await s.get(Note, r.id) for r in by_title.values()] + for row in fresh: + if row is not None: + row.arose_from_id = None + row.parent_id = None + await s.flush() + for row in fresh: + if row is not None: + await s.delete(row) + await s.commit() + async with async_session() as s: + user = await s.get(User, new_user_id) + if user is not None: + await s.delete(user) + await s.commit() + + +@pytest_asyncio.fixture(autouse=True) +async def _no_leftover_restored_user(): + """The payload's username is fixed, so a previous failed run would leave a + row that makes `restored` pick the wrong user. Clear it first.""" + async with async_session() as s: + stale = (await s.execute( + select(User).where(User.username == RESTORED_USERNAME) + )).scalars().all() + for user in stale: + notes = (await s.execute( + select(Note).where(Note.user_id == user.id) + )).scalars().all() + for n in notes: + n.arose_from_id = None + n.parent_id = None + await s.flush() + for n in notes: + await s.delete(n) + await s.delete(user) + await s.commit() + + +async def test_a_restored_record_keeps_what_it_IS(restored): + """#3182's headline. Without note_type and task_kind a restore reported + success and handed back a corpus where every snippet and process was a + plain note and every issue and spike was `work` — the whole vocabulary + milestone 312 and #3128 were about, gone, with nothing to notice it by.""" + by_title, _ = restored + + assert by_title["debounce — rate-limit"].note_type == "snippet" + assert by_title["DRY pass"].note_type == "process" + assert by_title["the fix"].task_kind == "issue" + assert by_title["the work that broke"].task_kind == "work" + + +async def test_a_restored_snippet_keeps_its_queryable_mirror(restored): + """The one field that would self-heal — backfill_snippet_data rebuilds it + from the body at startup — but a restore should not hand back a corpus + that needs a restart before it is findable by location.""" + by_title, _ = restored + assert by_title["debounce — rate-limit"].data == { + "name": "debounce", "language": "js", + } + + +async def test_the_provenance_edge_is_remapped_not_copied(restored): + """THE regression, and the reason this needed a real database. + + The payload's `arose_from_id` is an id in the SOURCE database. Copying it + through would leave the restored issue pointing at whatever record happens + to hold that number — a restore that succeeds and silently rewires + history. The edge must land on the RESTORED origin instead. + """ + by_title, src = restored + issue = by_title["the fix"] + origin = by_title["the work that broke"] + + assert issue.arose_from_id == origin.id + # ...and that is a different row from the one the payload named. + assert issue.arose_from_id != src["origin_id"] + + +async def test_description_and_status_survive(restored): + by_title, _ = restored + assert by_title["the fix"].description == "one-liner" + assert by_title["the fix"].status == "done" diff --git a/tests/test_integration_backup_rule_version_roundtrip.py b/tests/test_integration_backup_rule_version_roundtrip.py new file mode 100644 index 0000000..4a4b905 --- /dev/null +++ b/tests/test_integration_backup_rule_version_roundtrip.py @@ -0,0 +1,266 @@ +"""Real-Postgres round trip for rule_versions (milestone 323 step 1). + +Two things here cannot be shown with mocks, and both are the kind that fail +QUIETLY — a restore reports success and hands back history that is wrong. + +1. **`rule_id` is remapped, not copied.** It is an id in the SOURCE database. + A restore that writes it straight through succeeds and attaches every + snapshot to whatever rule happens to hold that number here — an edit + history filed against the wrong binding instruction, which is worse than + no history at all. This is #3182's `arose_from_id` trap on a new table. + +2. **A null actor does not drop the row.** `user_id` is SET NULL precisely so + history outlives the account that wrote it, so the restore deliberately + diverges from its `NoteVersion` sibling, which skips a version it cannot + map to a user. Nothing about the code says which of the two shapes is + intended; without this test, "make it match the sibling" reads as a tidy-up + and silently deletes the record the FK was chosen to preserve. + +These drive the REAL `restore_full_backup`. A test that re-derived the remap +would agree with whatever the product does, including nothing. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.rule_version import RuleVersion +from scribe.models.rulebook import Rule, Rulebook, RulebookTopic +from scribe.models.user import User +from scribe.services import backup +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +OWNER_USERNAME = "rule_version_roundtrip_owner" +RESTORED_USERNAME = "rule_version_roundtrip_restored" + + +async def _purge_books(username: str) -> None: + """user -> rulebook -> topic -> rule -> rule_version is ON DELETE CASCADE + the whole way down, and no ORM relationships are configured, so dropping + the books clears every row this file made under them.""" + async with async_session() as s: + users = (await s.execute( + select(User).where(User.username == username) + )).scalars().all() + for user in users: + books = (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == user.id) + )).scalars().all() + for book in books: + await s.delete(book) + await s.commit() + + +async def _purge_restored() -> None: + """The restored user is minted by the restore itself, so it goes too.""" + await _purge_books(RESTORED_USERNAME) + async with async_session() as s: + for user in (await s.execute( + select(User).where(User.username == RESTORED_USERNAME) + )).scalars().all(): + await s.delete(user) + await s.commit() + + +@pytest_asyncio.fixture(autouse=True) +async def _no_leftovers(): + """The usernames are fixed, so a previous failed run would leave rows that + make the fixtures below pick the wrong user — or hit `.one()` with two. + + SETUP ONLY, and that is not a stylistic choice. `_dispose_engine` is a + usefixtures entry, so it sets up AFTER this autouse one and therefore + tears down BEFORE it. Any database call here after a `yield` would open a + fresh pooled connection that the closing loop then orphans, and the next + test to touch Postgres dies on "Future attached to a different loop" — + including tests in other files. Cleanup belongs in the fixtures below, + whose teardowns run while the engine is still live. + """ + await _purge_restored() + await _purge_books(OWNER_USERNAME) + + +@pytest_asyncio.fixture +async def source(): + """One rule with two snapshots: one written by a user who still exists, + one whose actor is already gone. + + Both are needed. With only the attributed version, dropping unmappable + rows would pass; with only the orphaned one, so would dropping the actor + from every row. + """ + async with async_session() as s: + owner = await ensure_user(s, OWNER_USERNAME) + uid = owner.id + await s.commit() + + async with async_session() as s: + book = Rulebook(owner_user_id=uid, title="Environment facts") + s.add(book) + await s.flush() + topic = RulebookTopic(rulebook_id=book.id, title="ci") + s.add(topic) + await s.flush() + rule = Rule( + topic_id=topic.id, + title="The runner has no bash", + statement="Write every `run:` step in POSIX sh.", + verify_with="read the workflow's shell setting", + ) + s.add(rule) + await s.flush() + s.add_all([ + RuleVersion( + rule_id=rule.id, user_id=uid, + title="The runner has no bash", + statement="Use sh.", + why="the image ships no bash", + verify_with="read the workflow's shell setting", + tier="always_on", + ), + # The actor is already gone — what SET NULL leaves behind. + RuleVersion( + rule_id=rule.id, user_id=None, + title="The runner has no bash", + statement="Use POSIX sh in run steps.", + tier="always_on", + ), + ]) + await s.commit() + book_id, rule_id = book.id, rule.id + + async with async_session() as s: + user_rows = backup._user_rows( + [(await s.execute(select(User).where(User.id == uid))).scalars().one()] + ) + book_rows = backup._rulebook_rows( + [(await s.execute(select(Rulebook).where(Rulebook.id == book_id))) + .scalars().one()] + ) + topic_rows = backup._topic_rows( + (await s.execute( + select(RulebookTopic).where(RulebookTopic.rulebook_id == book_id) + )).scalars().all() + ) + rule_rows = backup._rule_rows( + [(await s.execute(select(Rule).where(Rule.id == rule_id))).scalars().one()] + ) + version_rows = backup._rule_version_rows( + (await s.execute( + select(RuleVersion).where(RuleVersion.rule_id == rule_id) + .order_by(RuleVersion.id) + )).scalars().all() + ) + # The restore mints a NEW user from the payload, so the restored corpus is + # separate from the source one — which is what makes the id assertion + # below able to fail. + user_rows[0]["username"] = RESTORED_USERNAME + + yield { + "payload": { + "version": backup.BACKUP_VERSION, + "users": user_rows, + "rulebooks": book_rows, + "rulebook_topics": topic_rows, + "rules": rule_rows, + "rule_versions": version_rows, + }, + "source_rule_id": rule_id, + "source_user_id": uid, + } + + async with async_session() as s: + book = await s.get(Rulebook, book_id) + if book is not None: + await s.delete(book) + await s.commit() + + +@pytest_asyncio.fixture +async def restored(source): + """Runs the real restore, then hands back the new rows. + + The restore mints a NEW user from the payload, so the restored corpus is + entirely separate from the source one — which is what makes the id + assertions below able to fail. + """ + await backup.restore_full_backup(source["payload"]) + async with async_session() as s: + user = (await s.execute( + select(User).where(User.username == RESTORED_USERNAME) + )).scalars().first() + assert user is not None, "the payload's user was not restored" + book = (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == user.id) + )).scalars().one() + topic = (await s.execute( + select(RulebookTopic).where(RulebookTopic.rulebook_id == book.id) + )).scalars().one() + rule = (await s.execute( + select(Rule).where(Rule.topic_id == topic.id) + )).scalars().one() + versions = (await s.execute( + select(RuleVersion).where(RuleVersion.rule_id == rule.id) + .order_by(RuleVersion.id) + )).scalars().all() + yield {"user": user, "rule": rule, "versions": versions, "source": source} + + # Here rather than in the autouse fixture: this teardown still runs while + # the engine is live. See _no_leftovers. + await _purge_restored() + + +async def test_both_snapshots_come_back(restored): + """The count first: everything below reads the same on an empty list, so + without this a restore that silently dropped both would look like a pass + on the shape assertions.""" + assert len(restored["versions"]) == 2 + + +async def test_the_history_attaches_to_the_RESTORED_rule(restored): + """#3182's trap. The source rule still exists and holds a different id, so + a straight-through copy would file this history against it — or against + whatever unrelated rule owns that number.""" + new_rule_id = restored["rule"].id + source_rule_id = restored["source"]["source_rule_id"] + assert new_rule_id != source_rule_id, ( + "the restore reused the source id, so this test cannot tell a remap " + "from a copy — the fixture is not proving what it claims" + ) + assert {v.rule_id for v in restored["versions"]} == {new_rule_id} + + +async def test_the_actor_is_remapped_to_the_restored_user(restored): + """`user_id` is an id in the source database too — the same trap, on the + column that answers "who changed this".""" + attributed = [v for v in restored["versions"] if v.user_id is not None] + assert len(attributed) == 1 + assert attributed[0].user_id == restored["user"].id + assert attributed[0].user_id != restored["source"]["source_user_id"] + + +async def test_a_snapshot_with_no_actor_survives(restored): + """The deliberate divergence from NoteVersion. `user_id` is SET NULL so + that history outlives the account that wrote it; skipping the row on an + unmappable user would throw away exactly what the FK preserves.""" + orphaned = [v for v in restored["versions"] if v.user_id is None] + assert len(orphaned) == 1, ( + "the version whose actor was already gone did not come back. A rule's " + "history is the only record of what it used to say — losing it " + "because nobody can be credited is the wrong trade." + ) + assert orphaned[0].statement == "Use POSIX sh in run steps." + + +async def test_the_text_survives(restored): + """The whole point of the table: what the rule USED to say. A restore that + kept the rows and lost their wording would preserve a changelog of empty + entries.""" + by_statement = {v.statement: v for v in restored["versions"]} + assert set(by_statement) == {"Use sh.", "Use POSIX sh in run steps."} + assert by_statement["Use sh."].why == "the image ships no bash" + assert by_statement["Use sh."].verify_with == ( + "read the workflow's shell setting" + ) + assert by_statement["Use sh."].tier == "always_on" diff --git a/tests/test_integration_rule_versions.py b/tests/test_integration_rule_versions.py new file mode 100644 index 0000000..83cc28e --- /dev/null +++ b/tests/test_integration_rule_versions.py @@ -0,0 +1,293 @@ +"""Real-Postgres tests for the rule-history write path (milestone 323 step 2). + +Every assertion here is about ORDERING or ABSENCE, which is why none of them +can be made with mocks. + +What a version is for: it holds what the rule USED TO SAY. The edit destroys +that, and nothing can recompute it — the rescoping of rule 79 had to be +hand-copied into a task log to survive (#3237), which is not a process, it is +a person remembering. + +Two of these pin a DELIBERATE ABSENCE. `note_versions` carries a 300-second +interval gate and a 50-version cap, both defences against note autosave. Rules +have no autosave, so here those guards would only ever discard a real edit. +Nothing in the code says so on its own, and "make it consistent with the +sibling" is a plausible-sounding change that would silently start dropping +history — so the absence is a test, not a comment. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.rulebook import Rule, Rulebook +from scribe.models.rule_version import RuleVersion +from scribe.services import rule_versions as rv_svc +from scribe.services import rulebooks as rulebooks_svc +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +OWNER_USERNAME = "rule_history_owner" + + +@pytest_asyncio.fixture +async def constraint(): + """One rule carrying a check, with no history yet. + + CLEANED UP AT SETUP, NOT TEARDOWN, and that is forced. `update_rule` fires + a detached `asyncio.create_task(upsert_rule_embedding(...))` that opens + its own connection and UPDATEs the rule row. A teardown that deleted the + rulebook would race it: the delete cascade-locks the rule the embedding + task is writing, and Postgres kills one of them with a deadlock. Purging + at setup instead runs on a fresh loop, after the previous test's loop + closed and cancelled whatever it left in flight. + """ + async with async_session() as s: + owner = await ensure_user(s, OWNER_USERNAME) + uid = owner.id + await s.commit() + # A previous failed run would leave a book whose topic title collides. + for book in (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == uid) + )).scalars().all(): + await s.delete(book) + await s.commit() + + book = await rulebooks_svc.create_rulebook(uid, "History fixtures") + topic = await rulebooks_svc.create_topic(book.id, uid, "ci") + rule = await rulebooks_svc.create_rule( + topic.id, uid, "The runner has no bash", + "Write every `run:` step in POSIX sh.", + why="the image ships no bash", + verify_with="read the workflow's shell setting", + ) + + return {"uid": uid, "rule_id": rule.id} + + +async def _versions(rule_id: int) -> list[RuleVersion]: + async with async_session() as s: + return list((await s.execute( + select(RuleVersion).where(RuleVersion.rule_id == rule_id) + .order_by(RuleVersion.id) + )).scalars().all()) + + +async def test_an_edit_leaves_the_text_it_replaced(constraint): + """The headline. The version holds the OLD statement — the rule row + already holds the new one, so a snapshot of the new text would record + nothing that was not already there.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], + statement="Write every `run:` step in POSIX sh, and set shell: sh.", + ) + [version] = await _versions(constraint["rule_id"]) + assert version.statement == "Write every `run:` step in POSIX sh." + assert version.why == "the image ships no bash" + + +async def test_the_actor_is_recorded(constraint): + """`user_id` is who made the edit — the question an audit trail over a + binding instruction is actually asked.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], title="The runner ships no bash", + ) + [version] = await _versions(constraint["rule_id"]) + assert version.user_id == constraint["uid"] + + +async def test_an_edit_that_changes_no_text_writes_nothing(constraint): + """Git semantics. `update_rule` is reached by callers that resend every + field, so without the skip a form saved twice would file an identical + snapshot and the history would stop reading as a list of edits.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], + statement="Write every `run:` step in POSIX sh.", + title="The runner has no bash", + ) + assert await _versions(constraint["rule_id"]) == [] + + +async def test_reordering_is_not_an_edit(constraint): + """`order_index` goes through the same door but is not text. A run of + "moved rule 4 above rule 3" snapshots would bury the edits somebody is + looking for.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], order_index=7, + ) + assert await _versions(constraint["rule_id"]) == [] + + +async def test_the_snapshot_holds_the_CHECK_that_was_in_force(constraint): + """The ordering trap. `update_rule` drops `verified_at` when `verify_with` + changes, and rewriting the check is exactly the edit whose history matters + most — so the snapshot has to be taken before the field loop, or it + records the new check against the old wording.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], + verify_with="run `sh -c 'echo $0'` in a job step", + ) + [version] = await _versions(constraint["rule_id"]) + assert version.verify_with == "read the workflow's shell setting" + + async with async_session() as s: + rule = await s.get(Rule, constraint["rule_id"]) + assert rule.verify_with == "run `sh -c 'echo $0'` in a job step" + + +async def test_clearing_a_field_is_an_edit_worth_recording(constraint): + """A rule that stops being a constraint loses its check entirely, and the + version is then the only place the retired check exists. `clear` is a + separate argument from the field loop — a snapshot wired to only one of + the two would lose exactly this case.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], clear=("verify_with",), + ) + [version] = await _versions(constraint["rule_id"]) + assert version.verify_with == "read the workflow's shell setting" + + async with async_session() as s: + rule = await s.get(Rule, constraint["rule_id"]) + assert rule.verify_with is None + + +async def test_rapid_successive_edits_are_all_kept(constraint): + """DELIBERATE ABSENCE — no MIN_VERSION_INTERVAL_SECONDS. + + `note_versions` skips a snapshot taken within 300 seconds of the last one, + because note autosave fires every 60 and would burn all 50 slots. Rules + have no autosave: every one of these is a deliberate update_rule, and + three edits in the same second are three edits. Porting the sibling's gate + here would silently keep only the first. + """ + for n in (1, 2, 3): + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement=f"revision {n}", + ) + versions = await _versions(constraint["rule_id"]) + assert [v.statement for v in versions] == [ + "Write every `run:` step in POSIX sh.", "revision 1", "revision 2", + ] + + +async def test_the_service_carries_no_pruning_machinery(constraint): + """DELIBERATE ABSENCE — no MAX_VERSIONS and no pruning DELETE. + + Asserted structurally because the alternative is 51 real edits to prove a + negative. The cap exists in `note_versions` to bound autosave volume; a + rule is edited a handful of times in its life, so a cap here could only + ever discard the one edit somebody went looking for. + """ + assert not hasattr(rv_svc, "MAX_VERSIONS") + assert not hasattr(rv_svc, "MIN_VERSION_INTERVAL_SECONDS") + + +async def test_history_reads_newest_first(constraint): + """The reader's question is "what did this just say?", so the most recent + supersession has to be the first row.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement="first change", + ) + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement="second change", + ) + listed = await rv_svc.list_versions(constraint["rule_id"]) + assert [v.statement for v in listed] == [ + "first change", "Write every `run:` step in POSIX sh.", + ] + + +# ── The read path is ACL-scoped (milestone 323 step 3) ───────────────── +# +# Rule 47: every read of user data is scoped by owner. A version carries a +# `user_id`, which makes it tempting to scope the history by it — that would +# be wrong in both directions, and these pin which way round it goes. + + +@pytest_asyncio.fixture +async def stranger(): + """A second user who owns nothing in the fixture above.""" + async with async_session() as s: + other = await ensure_user(s, "rule_history_stranger") + uid = other.id + await s.commit() + return uid + + +async def test_the_owner_reads_the_history(constraint): + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement="reworded", + ) + versions = await rulebooks_svc.list_rule_versions( + constraint["rule_id"], constraint["uid"], + ) + assert [v.statement for v in versions] == [ + "Write every `run:` step in POSIX sh.", + ] + + +async def test_a_stranger_reads_nothing(constraint, stranger): + """None, not an empty list. The two mean different things — "not your + rule" versus "this rule has never been reworded" — and collapsing them + would tell a caller the rule exists and is unedited.""" + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement="reworded", + ) + assert await rulebooks_svc.list_rule_versions( + constraint["rule_id"], stranger, + ) is None + + +async def test_a_version_cannot_be_read_through_a_DIFFERENT_rule(constraint): + """The check and the fetch have to agree about which rule is in play. + + The second rule is owned by the SAME user on purpose — an id that simply + does not exist would pass on the ownership check alone and prove nothing + about the `rule_id` clause. Here the caller genuinely owns the rule they + name and genuinely owns the version's rule, and the read must still + refuse, because scoping by version id alone is how a snapshot leaks + through whichever rule the caller happens to be able to see. + """ + await rulebooks_svc.update_rule( + constraint["rule_id"], constraint["uid"], statement="reworded", + ) + [version] = await _versions(constraint["rule_id"]) + + async with async_session() as s: + rule = await s.get(Rule, constraint["rule_id"]) + topic_id = rule.topic_id + sibling = await rulebooks_svc.create_rule( + topic_id, constraint["uid"], "A different rule", "Unrelated.", + ) + + assert await rulebooks_svc.get_rule_version( + sibling.id, version.id, constraint["uid"], + ) is None + # And through its own rule it reads fine — or the assertion above would + # pass for the wrong reason. + assert await rulebooks_svc.get_rule_version( + constraint["rule_id"], version.id, constraint["uid"], + ) is not None + + +async def test_the_actor_does_not_grant_the_read(constraint, stranger): + """The inverse mistake. `user_id` on a version is the ACTOR, so scoping + the history by it would hand someone the snapshots they personally wrote + on a rule that has since moved out of their reach — and would hide, from + the rule's owner, every edit somebody else made.""" + async with async_session() as s: + s.add(RuleVersion( + rule_id=constraint["rule_id"], user_id=stranger, + title="written by someone else", statement="a stranger's edit", + )) + await s.commit() + + assert await rulebooks_svc.list_rule_versions( + constraint["rule_id"], stranger, + ) is None + + owner_view = await rulebooks_svc.list_rule_versions( + constraint["rule_id"], constraint["uid"], + ) + assert "a stranger's edit" in [v.statement for v in owner_view] diff --git a/tests/test_knowledge_facets.py b/tests/test_knowledge_facets.py new file mode 100644 index 0000000..3e5f481 --- /dev/null +++ b/tests/test_knowledge_facets.py @@ -0,0 +1,120 @@ +"""The type facet, in both of its dialects. + +The facet predicate is written twice by necessity — as SQL for rows the +database hands back, and as Python for candidates the vector search has +already fetched — plus a third time as the `is_task` pre-filter that narrows +the candidate set before it exists. Those three used to be hand-written and +agreed only by luck: adding `issue` to the SQL arm alone would have set the +pre-filter to is_task=False, handed the Python arm a candidate set with no +tasks in it, and returned an empty semantic half for the Issues facet forever +with nothing red anywhere (#3128). + +They are now generated from one table. These tests pin the property that made +the trap possible, so it cannot come back by a different route. +""" +import pytest + +from scribe.services.knowledge import ( + _FACETS, + FACET_TYPES, + NON_TASK_FACETS, + _apply_type_filter, + facet_is_task, + matches_facet, +) +from tests.helpers import compiled_sql, fake_note, fake_snippet, fake_task + + +def _sql(note_type): + from sqlalchemy import select + + from scribe.models.note import Note + + return compiled_sql(_apply_type_filter(select(Note.id), note_type)) + + +# One representative row per shape the corpus actually holds. +ROWS = { + "plain note": fake_note(note_type="note"), + "process": fake_note(note_type="process"), + "snippet": fake_snippet(), + "work task": fake_task(task_kind="work", note_type="note"), + "issue": fake_task(task_kind="issue", note_type="note"), + "spike": fake_task(task_kind="spike", note_type="note"), + "legacy plan": fake_task(task_kind="plan", note_type="note"), +} + + +@pytest.mark.parametrize("facet", sorted(FACET_TYPES)) +def test_pre_filter_never_excludes_a_row_the_facet_wants(facet): + """THE regression. `facet_is_task` narrows the semantic candidate set before + `matches_facet` ever sees it, so a pre-filter that disagrees with the + predicate doesn't return wrong rows — it returns NO rows, silently, on one + half of a hybrid search.""" + want = facet_is_task(facet) + for label, row in ROWS.items(): + if matches_facet(row, facet): + assert want is None or want == row.is_task, ( + f"facet {facet!r} accepts the {label} row, but its pre-filter " + f"asks for is_task={want} while the row has is_task={row.is_task} " + f"— the semantic arm would never be handed this row" + ) + + +@pytest.mark.parametrize( + "facet,expected", + [ + ("task", {"work task", "issue", "spike", "legacy plan"}), + ("issue", {"issue"}), + ("spike", {"spike"}), + ("work", {"work task"}), + ("plan", {"legacy plan"}), + ("note", {"plain note"}), + ("process", {"process"}), + ("snippet", {"snippet"}), + ("", set(ROWS)), + ], +) +def test_each_facet_selects_exactly_its_own_rows(facet, expected): + assert {k for k, row in ROWS.items() if matches_facet(row, facet)} == expected + + +def test_a_plain_note_is_not_selected_by_its_own_type_when_it_is_a_task(): + """A task's `note_type` is 'note' — that column says nothing about task-ness. + The hand-written Python arm omitted the `status IS NULL` half its SQL twin + carried, so it only avoided returning every task under the Notes facet + because the pre-filter had already dropped them.""" + assert matches_facet(fake_task(note_type="note"), "note") is False + + +def test_an_unknown_facet_matches_nothing_rather_than_everything(): + """A typo must return an empty list, never the whole corpus.""" + assert all(not matches_facet(row, "wrok") for row in ROWS.values()) + assert "notes.status IS NULL" in _sql("wrok") + + +def test_the_live_task_kinds_are_all_facets(): + """`issue` shipped in 0065 and `spike` in 0091; the browse vocabulary went + three kinds without noticing either.""" + for kind in ("work", "issue", "spike"): + assert kind in FACET_TYPES and _FACETS[kind][0] is True + + +def test_non_task_facets_are_the_note_types_and_only_those(): + assert set(NON_TASK_FACETS) == {"note", "process", "snippet"} + + +@pytest.mark.parametrize("facet", sorted(FACET_TYPES)) +def test_sql_arm_constrains_the_axis_the_facet_lives_on(facet): + """The SQL dialect of the same table. A task facet must pin `status` (and, + for a single kind, `task_kind`); a record-type facet must pin `note_type` + AND exclude tasks.""" + sql = _sql(facet) + is_task, value = _FACETS[facet] + assert "notes.deleted_at IS NULL" in sql + if is_task: + assert "notes.status IS NOT NULL" in sql + assert (f"notes.task_kind = '{value}'" in sql) is (value is not None) + else: + assert "notes.status IS NULL" in sql + assert f"notes.note_type = '{value}'" in sql diff --git a/tests/test_mcp_tool_notes.py b/tests/test_mcp_tool_notes.py index 7a6d2f3..6637f25 100644 --- a/tests/test_mcp_tool_notes.py +++ b/tests/test_mcp_tool_notes.py @@ -1,5 +1,5 @@ """Tests for fable_*_note tools.""" -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -214,17 +214,53 @@ async def test_create_note_project_zero_becomes_none(): @pytest.mark.asyncio async def test_update_note_only_sends_non_default_fields(): """Omitted (default) fields must NOT reach the service — otherwise they'd - overwrite real data with empty strings.""" + overwrite real data with empty strings. + + `clear` is always forwarded and is not a field: it is how this door says + "unset these", and an empty one says "unset nothing". Same shape the rules + door took in #3096, and the same test that had to learn about it there.""" fake = fake_note() mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock): await update_note(note_id=1, title="new title") - # Service got user_id, note_id positional + only the title kwarg args, kwargs = mock.call_args assert args == (7, 1) + assert kwargs.pop("clear") == (), "nothing was asked to be cleared" assert kwargs == {"title": "new title"} +@pytest.mark.asyncio +async def test_update_note_forwards_a_check_but_not_an_empty_one(): + """"" means "leave this alone" at this door — an agent updating a body must + not wipe a check it was never asked about (milestone 317).""" + mock = AsyncMock(return_value=fake_note()) + with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock): + await update_note(note_id=1, verify_with="curl the docs") + assert mock.call_args.kwargs["verify_with"] == "curl the docs" + + with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock): + await update_note(note_id=1, title="t") + assert "verify_with" not in mock.call_args.kwargs + + +@pytest.mark.asyncio +async def test_update_note_forwards_clear_so_a_check_can_be_removed(): + """The only way to unset a field at a door where "" means "leave alone".""" + mock = AsyncMock(return_value=fake_note()) + with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock): + await update_note(note_id=1, clear=["verify_with"]) + assert mock.call_args.kwargs["clear"] == ["verify_with"] + + +@pytest.mark.asyncio +async def test_create_note_forwards_the_check_fields(): + mock = AsyncMock(return_value=fake_note()) + with patch("scribe.mcp.tools.notes.notes_svc.create_note", mock): + await create_note(title="t", verify_with="curl", expires_when="AMO changes") + assert mock.call_args.kwargs["verify_with"] == "curl" + assert mock.call_args.kwargs["expires_when"] == "AMO changes" + + @pytest.mark.asyncio async def test_update_note_empty_tags_clears_explicitly(): """tags=[] is an explicit clear, distinct from tags=None (omit).""" @@ -232,7 +268,9 @@ async def test_update_note_empty_tags_clears_explicitly(): mock = AsyncMock(return_value=fake) with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock): await update_note(note_id=1, tags=[]) - assert mock.call_args.kwargs == {"tags": []} + kwargs = dict(mock.call_args.kwargs) + kwargs.pop("clear") + assert kwargs == {"tags": []} @pytest.mark.asyncio @@ -256,12 +294,37 @@ async def test_update_note_raises_when_not_found(): @pytest.mark.asyncio async def test_delete_note_soft_deletes_and_returns_batch(): + doomed = MagicMock() + doomed.title = "the note that went" with patch( "scribe.mcp.tools.notes.trash_svc.delete", AsyncMock(return_value="batch-1"), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=(doomed, "owner")), ): result = await delete_note(note_id=7) assert result["deleted_batch_id"] == "batch-1" + # The confirmation NAMES what went (#3273). After the delete the row is + # trashed, so this line is the last chance to say which note it was. + assert result["title"] == "the note that went" + assert "the note that went" in result["message"] + + +@pytest.mark.asyncio +async def test_delete_note_still_deletes_when_the_title_lookup_fails(): + """The title is a courtesy on top of the delete, never a precondition for + it. A lookup that errors costs the name, not the operation.""" + with patch( + "scribe.mcp.tools.notes.trash_svc.delete", + AsyncMock(return_value="batch-1"), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(side_effect=RuntimeError("database down")), + ): + result = await delete_note(note_id=7) + assert result["deleted_batch_id"] == "batch-1" + assert result["title"] == "" @pytest.mark.asyncio @@ -269,6 +332,9 @@ async def test_delete_note_raises_when_not_found(): with patch( "scribe.mcp.tools.notes.trash_svc.delete", AsyncMock(return_value=None), + ), patch( + "scribe.mcp.tools.notes.notes_svc.get_note_for_user", + AsyncMock(return_value=None), ): with pytest.raises(ValueError, match="note 999 not found"): await delete_note(note_id=999) diff --git a/tests/test_mcp_tool_rulebooks.py b/tests/test_mcp_tool_rulebooks.py index c922a06..8b96917 100644 --- a/tests/test_mcp_tool_rulebooks.py +++ b/tests/test_mcp_tool_rulebooks.py @@ -229,8 +229,9 @@ def test_register_attaches_every_tool(): mcp = FakeMCP() register(mcp) - # 26 through milestone 307, +2 for the staleness sweep (milestone 312). - assert len(mcp.names) == 28 + # 26 through milestone 307, +2 for the staleness sweep (milestone 312), + # +1 for a rule's edit history (milestone 323). + assert len(mcp.names) == 29 # spot-check a few names assert "list_rulebooks" in mcp.names assert "create_rule" in mcp.names @@ -244,6 +245,8 @@ def test_register_attaches_every_tool(): # milestone 312: the sweep, and the stamp that answers it assert "rules_due_for_verification" in mcp.names assert "mark_rule_verified" in mcp.names + # milestone 323: what a rule used to say + assert "rule_history" in mcp.names assert "unsuppress_rule_for_project" in mcp.names assert "suppress_topic_for_project" in mcp.names assert "unsuppress_topic_for_project" in mcp.names @@ -257,7 +260,10 @@ async def test_list_always_on_rules_returns_empty_when_no_always_on_rulebooks(): ): from scribe.mcp.tools.rulebooks import list_always_on_rules out = await list_always_on_rules() - assert out == {"rules": [], "total": 0} + # An install with no always-on rulebooks still gets a marker (milestone + # 323): "no rules" is a STATE, and a payload that omitted the key would + # make the write path read every session on a fresh install as a change. + assert out == {"rules": [], "total": 0, "rules_etag": "empty|0"} @pytest.mark.asyncio @@ -433,3 +439,125 @@ async def test_unrelate_rules_raises_when_the_edge_is_gone(): from scribe.mcp.tools.rulebooks import unrelate_rules with pytest.raises(ValueError, match="not found"): await unrelate_rules(relation_id=99) + + +# ── rule_history (milestone 323 step 3) ──────────────────────────────── + +def _fake_version(**over): + """A RuleVersion-shaped stand-in. A real model instance rather than a + MagicMock, because the tool calls `to_dict` and a mock would hand back + another mock instead of failing.""" + from scribe.models.rule_version import RuleVersion + from datetime import datetime, timezone + + defaults = { + "id": 5, "rule_id": 100, "user_id": 1, + "title": "The runner has no bash", "statement": "Use sh.", + "why": "the image ships no bash", "how_to_apply": None, + "when_to_apply": None, "tier": "always_on", + "verify_with": "read the workflow's shell setting", + "expires_when": None, + "created_at": datetime(2026, 8, 29, tzinfo=timezone.utc), + } + return RuleVersion(**{**defaults, **over}) + + +@pytest.mark.asyncio +async def test_rule_history_lists_without_the_heavy_text(): + """The listing answers "when, and by whom". A rule's statement runs to + thousands of characters, so a history carrying every field would cost + more to read than the answer is worth.""" + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions", + AsyncMock(return_value=[_fake_version(), _fake_version(id=4)]), + ), patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=fake_rule( + id=100, title="The runner has no bash", statement="s", topic_id=10, + )), + ): + from scribe.mcp.tools.rulebooks import rule_history + out = await rule_history(rule_id=100) + + assert out["total"] == 2 + assert out["versions"][0]["title"] == "The runner has no bash" + assert "statement" not in out["versions"][0] + assert "why" not in out["versions"][0] + + +@pytest.mark.asyncio +async def test_rule_history_opens_one_version_in_full(): + """Passing a version id switches from the index to the text — which is + the whole reason the listing can afford to omit it.""" + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule_version", + AsyncMock(return_value=_fake_version()), + ): + from scribe.mcp.tools.rulebooks import rule_history + out = await rule_history(rule_id=100, version_id=5) + + assert out["statement"] == "Use sh." + assert out["verify_with"] == "read the workflow's shell setting" + + +@pytest.mark.asyncio +async def test_rule_history_says_an_empty_history_is_ordinary(): + """Most rules have never been reworded, and nothing was written before + milestone 323. Without this line an empty list reads as a lost history or + a broken tool.""" + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions", + AsyncMock(return_value=[]), + ), patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule", + AsyncMock(return_value=fake_rule( + id=100, title="The runner has no bash", statement="s", topic_id=10, + )), + ): + from scribe.mcp.tools.rulebooks import rule_history + out = await rule_history(rule_id=100) + + assert out["total"] == 0 + assert "never been reworded" in out["note"] + + +@pytest.mark.asyncio +async def test_rule_history_raises_when_the_rule_is_not_yours(): + """None from the service means "not readable", and the tool must not + turn that into an empty history — which would read as "this rule has no + past" rather than "this is not your rule".""" + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.list_rule_versions", + AsyncMock(return_value=None), + ): + from scribe.mcp.tools.rulebooks import rule_history + with pytest.raises(ValueError, match="rule 100 not found"): + await rule_history(rule_id=100) + + +@pytest.mark.asyncio +async def test_rule_history_raises_for_a_version_on_another_rule(): + with patch( + "scribe.mcp.tools.rulebooks.rulebooks_svc.get_rule_version", + AsyncMock(return_value=None), + ): + from scribe.mcp.tools.rulebooks import rule_history + with pytest.raises(ValueError, match="version 5 not found"): + await rule_history(rule_id=100, version_id=5) + + +def test_rule_history_docstring_says_what_a_version_HOLDS(): + """The one thing a reader gets wrong unaided: an entry is the text the + edit REPLACED, not the text it introduced. Read the other way, every + diff comes out backwards — so the docstring has to say it, and this is + the guard against a later tidy-up dropping the line.""" + from scribe.mcp.tools.rulebooks import rule_history + + doc = rule_history.__doc__ or "" + assert "REPLACED" in doc + assert "no restore" in doc.lower(), ( + "the docstring no longer explains that a rule version cannot be " + "restored. A caller who assumes a revert exists will look for one " + "and, not finding it, is likely to hand-copy the old text back with " + "no record of why." + ) diff --git a/tests/test_mcp_tool_snippets.py b/tests/test_mcp_tool_snippets.py index 7773f14..c7bb5b5 100644 --- a/tests/test_mcp_tool_snippets.py +++ b/tests/test_mcp_tool_snippets.py @@ -138,9 +138,18 @@ async def test_merge_snippets_read_only_target_says_why(): @pytest.mark.asyncio async def test_delete_snippet_retires_or_raises(): from scribe.mcp.tools.snippets import delete_snippet - with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=True)): - assert await delete_snippet(1) == {"deleted": True, "id": 1} - with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=False)): + + doomed = MagicMock() + doomed.title = "debounce helper" + # The confirmation names what went (#3273) — after the delete there is + # nothing left to look the title up from. + with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=True)), \ + patch("scribe.services.snippets.get_snippet", AsyncMock(return_value=doomed)): + assert await delete_snippet(1) == { + "deleted": True, "id": 1, "title": "debounce helper", + } + with patch("scribe.services.snippets.delete_snippet", AsyncMock(return_value=False)), \ + patch("scribe.services.snippets.get_snippet", AsyncMock(return_value=None)): with pytest.raises(ValueError): await delete_snippet(404) diff --git a/tests/test_records_are_named_not_numbered.py b/tests/test_records_are_named_not_numbered.py new file mode 100644 index 0000000..08ecdd5 --- /dev/null +++ b/tests/test_records_are_named_not_numbered.py @@ -0,0 +1,106 @@ +"""A record is cited by id AND title — never by number alone. + +THE PROBLEM THIS IS ABOUT. The agent has the record open; the operator does +not. `#3244` reads as complete to the writer and as homework to the reader, +who has to look it up to know what their own conversation is about. The +operator's words: *"I don't know what a note, task, or milestone is by its ID +number."* + +PRODUCT, NOT A RULE (rule 119). Every Scribe user hits this, so the fix is in +the surfaces the product ships — the skill that shapes how an agent writes, +and the tool responses that hand a record back. A per-instance rule would fix +it for one operator and leave the behaviour wrong for everyone else. + +Scribe's duplicate gate already had the right shape — `id 412: "debounce +helper"` — which is why these assert the CONVENTION reaches the other +surfaces rather than inventing a new one. +""" +import pathlib + +import pytest +from unittest.mock import AsyncMock, MagicMock, patch + +SKILL = ( + pathlib.Path(__file__).resolve().parents[1] + / "plugin/skills/using-scribe/SKILL.md" +) + + +def test_the_skill_carries_the_convention(): + """The skill is read while deciding HOW to write, which is the only moment + this can be applied. A tool response can name one record; only the skill + can govern the prose around it.""" + text = " ".join(SKILL.read_text().split()) + assert "Name the record, never just its number" in text, ( + "the using-scribe skill no longer tells an agent to write the title " + "alongside the id. Nothing else governs how records are cited in " + "prose, commit messages, or task bodies." + ) + + +def test_the_skill_says_where_it_matters_most(): + """A convention stated only for chat messages gets applied only there — + and the places read LATER, by someone with even less context, are where a + bare id costs most.""" + text = " ".join(SKILL.read_text().split()).lower() + assert "commit message" in text and "task bod" in text + + +def test_the_skill_says_to_look_up_a_title_it_does_not_know(): + """The escape hatch that would otherwise swallow the convention whole: an + agent that does not know the title will emit the number and move on.""" + text = " ".join(SKILL.read_text().split()).lower() + assert "an id you can't name is one you haven't checked" in text + + +# ── The tool responses (the other half) ──────────────────────────────── +# +# A deletion is the sharpest case: afterwards the row is trashed, so if the +# confirmation did not name it, nothing can. An operator who cannot recognise +# what was deleted cannot tell it was the wrong thing. + +@pytest.mark.parametrize("module,fn,kind,rid", [ + ("tasks", "delete_task", "task", 3244), + ("notes", "delete_note", "note", 2109), + ("milestones", "delete_milestone", "milestone", 323), +]) +@pytest.mark.asyncio +async def test_a_delete_confirmation_names_what_it_deleted( + module, fn, kind, rid, monkeypatch, +): + import importlib + + mod = importlib.import_module(f"scribe.mcp.tools.{module}") + title = "the staleness signal" + row = MagicMock() + row.title = title + + patches = [ + patch.object(mod, "current_user_id", MagicMock(return_value=1)), + patch.object(mod.trash_svc, "delete", AsyncMock(return_value="batch-1")), + ] + if module == "milestones": + patches.append( + patch.object(mod.milestones_svc, "get_milestone", + AsyncMock(return_value=row))) + else: + patches.append( + patch.object(mod.notes_svc, "get_note_for_user", + AsyncMock(return_value=(row, "owner")))) + for p in patches: + p.start() + try: + out = await getattr(mod, fn)(rid) + finally: + for p in patches: + p.stop() + + assert out["title"] == title, f"{fn} returns no title for the record" + assert title in out["message"], ( + f"{fn}'s message names only the id. After the delete the row is " + f"trashed, so this line is the last chance to say WHAT went." + ) + assert str(rid) in out["message"], ( + f"{fn} dropped the id — the title alone is not addressable, and the " + f"convention is id AND title, not one or the other." + ) diff --git a/tests/test_recurrence.py b/tests/test_recurrence.py index bb89bfc..2a26aca 100644 --- a/tests/test_recurrence.py +++ b/tests/test_recurrence.py @@ -1,13 +1,23 @@ -"""Tests for task lifecycle timestamps and recurrence logic.""" +"""Tests for task lifecycle timestamps and recurrence logic. + +The note stand-ins come from `fake_note`, not a bare MagicMock. These four +predated that helper and broke the moment `update_note` started reading a +column they did not set (milestone 317): a fresh MagicMock is truthy on every +attribute, so `verify_with` read as "this record carries a check" and the +guard refused the write. That is note 2109's lesson, and the reason fake_note +exists — a stand-in has to be able to say NO. +""" from datetime import date, datetime, timezone from unittest.mock import AsyncMock, MagicMock, patch +from tests.helpers import fake_note, make_mock_session + # ── Timestamp side-effect tests ────────────────────────────────────────────── async def test_update_note_sets_started_at_on_in_progress(): """started_at is set when status transitions to in_progress.""" - mock_note = MagicMock() + mock_note = fake_note() mock_note.status = "in_progress" mock_note.started_at = None mock_note.recurrence_rule = None @@ -28,7 +38,7 @@ async def test_update_note_sets_started_at_on_in_progress(): async def test_update_note_sets_completed_at_on_done(): """completed_at is set when status transitions to done.""" - mock_note = MagicMock() + mock_note = fake_note() mock_note.status = "done" mock_note.started_at = datetime(2026, 3, 1, tzinfo=timezone.utc) mock_note.completed_at = None @@ -50,7 +60,7 @@ async def test_update_note_sets_completed_at_on_done(): async def test_update_note_clears_timestamps_on_todo(): """started_at and completed_at are cleared when status resets to todo.""" - mock_note = MagicMock() + mock_note = fake_note() mock_note.status = "todo" mock_note.started_at = datetime(2026, 3, 1, tzinfo=timezone.utc) mock_note.completed_at = datetime(2026, 3, 15, tzinfo=timezone.utc) @@ -74,7 +84,7 @@ async def test_update_note_clears_timestamps_on_todo(): async def test_update_note_preserves_started_at_if_already_set(): """started_at is not overwritten on a second transition to in_progress.""" original_start = datetime(2026, 3, 1, tzinfo=timezone.utc) - mock_note = MagicMock() + mock_note = fake_note() mock_note.status = "in_progress" mock_note.started_at = original_start mock_note.recurrence_rule = None @@ -96,7 +106,6 @@ async def test_update_note_preserves_started_at_if_already_set(): # ── Recurrence rule validation ──────────────────────────────────────────────── import pytest -from tests.helpers import make_mock_session def test_validate_interval_rule_valid(): diff --git a/tests/test_rules_etag.py b/tests/test_rules_etag.py new file mode 100644 index 0000000..151751a --- /dev/null +++ b/tests/test_rules_etag.py @@ -0,0 +1,267 @@ +"""The staleness marker on the rules payload (milestone 323 step 5). + +WHAT THE MARKER IS FOR: telling a session that the rules it is holding have +MOVED since it loaded them. Not general staleness — the limitation is stated +in services/rulebooks.py and in the write-path arm, and two tests here pin the +cases that would otherwise be quietly lost. + +The two that matter most are both about NOT crying wolf. A marker that reports +a change when nothing changed gets ignored within a day, and an ignored +staleness signal is worse than none: it trains a reader to skip the line that +will one day be true. +""" +from datetime import datetime, timedelta, timezone +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services import rulebooks as svc + +NOW = datetime(2026, 8, 30, 12, 0, tzinfo=timezone.utc) +LATER = NOW + timedelta(hours=1) + + +def _rule(updated_at=NOW, rid=1, title="a rule"): + """A Rule-shaped stand-in. The marker only ever reads three attributes, + and a real model would need a session to build.""" + return SimpleNamespace(id=rid, title=title, updated_at=updated_at) + + +def test_the_same_set_produces_the_same_marker(): + """The whole mechanism rests on this. If the marker moved on its own, every + write would report a change and the line would be noise by lunchtime.""" + rules = [_rule(rid=1), _rule(rid=2, updated_at=LATER)] + assert svc.rules_etag(rules) == svc.rules_etag(list(reversed(rules))), ( + "the marker depends on the ORDER rules come back in, so any query " + "whose sort changes would look like an edit" + ) + + +def test_an_edit_moves_the_marker(): + before = svc.rules_etag([_rule(rid=1), _rule(rid=2)]) + after = svc.rules_etag([_rule(rid=1), _rule(rid=2, updated_at=LATER)]) + assert before != after + + +def test_a_DELETED_rule_moves_the_marker(): + """THE CASE max(updated_at) ALONE CANNOT SEE, and the reason the count is + in there. Deleting a rule moves no timestamp — and it is the single change + that takes an instruction OUT of force, which is the one a session most + needs to hear about.""" + before = svc.rules_etag([_rule(rid=1), _rule(rid=2)]) + after = svc.rules_etag([_rule(rid=1)]) + assert before != after, ( + "a deleted rule left the marker unchanged — the count is missing, and " + "the session would keep obeying an instruction that no longer exists" + ) + + +def test_no_rules_is_a_state_not_a_change(): + """Rule 115: this has to behave on an install with no rules at all. `max()` + over an empty set raises; a marker that raised would take the whole write + path's hint down, and one that varied would tell every session on a fresh + install that its rules had changed.""" + assert svc.rules_etag([]) == svc.rules_etag([]) + assert svc.rules_etag([]) != svc.rules_etag([_rule()]) + + +def test_moved_since_names_only_what_actually_moved(): + held = svc.rules_etag([_rule(rid=1), _rule(rid=2)]) + current = [_rule(rid=1), _rule(rid=2, updated_at=LATER, title="reworded")] + moved = svc.rules_moved_since(current, held) + assert [r.id for r in moved] == [2] + + +def test_a_matching_marker_names_nothing(): + rules = [_rule(rid=1), _rule(rid=2)] + assert svc.rules_moved_since(rules, svc.rules_etag(rules)) == [] + + +def test_an_unreadable_marker_reports_no_change(): + """A caller cannot act on "something differs but I cannot say what", and a + garbled marker must never be rendered as a change — that is the shape of a + signal that gets ignored.""" + assert svc.rules_moved_since([_rule(updated_at=LATER)], "not-an-etag") == [] + assert svc.rules_moved_since([_rule(updated_at=LATER)], "") == [] + assert svc.etag_count("garbled") is None + + +def test_the_empty_marker_reports_no_change(): + """An install that had no rules and now has some: the count says so, and + this function has no timestamp to reason from. Silence here, not a claim.""" + assert svc.rules_moved_since([_rule()], svc.rules_etag([])) == [] + + +def test_the_count_survives_the_round_trip(): + assert svc.etag_count(svc.rules_etag([_rule(rid=1), _rule(rid=2)])) == 2 + assert svc.etag_count(svc.rules_etag([])) == 0 + + +def test_the_limitation_is_stated_where_a_reader_will_be(): + """A future reader who finds an etag will assume it covers staleness + generally. It does not — it is blind to compaction, which is the most + common case — and the SessionStart nudge is that case's only mechanism. + + Pinned because the plausible mistake is retiring a nudge that works on the + strength of a signal that does not cover it, and the comment is the only + thing standing in the way. + """ + import inspect + + from scribe.services import plugin_context + + for module in (svc, plugin_context): + src = inspect.getsource(module).lower() + assert "compaction" in src and "etag" in src, ( + f"{module.__name__} no longer explains what the rules marker " + f"cannot see. Without it the next reader will treat an etag as a " + f"general staleness check and soften the SessionStart nudge." + ) + + +# ── The arm that delivers the message (milestone 323 step 5) ─────────── +# +# The marker is worth nothing until a session is actually TOLD. These drive +# the real `build_write_path_hint`, because the feature IS a line arriving in +# a hook's output — a test of the helper alone would prove the arithmetic and +# nothing about the delivery. + + +def _quiet_write_path(pc, rules): + """Every other arm stubbed to silent, so the only line that can appear is + the one under test.""" + return ( + patch.object(pc, "get_writepath_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.6, + "top_k": 3})), + patch.object(pc.snippets_svc, "list_snippets", + AsyncMock(return_value=([], 0))), + patch.object(pc, "semantic_search_notes", AsyncMock(return_value=[])), + patch.object(pc, "semantic_search_rules", AsyncMock(return_value=[])), + patch.object(pc, "record_retrieval", MagicMock()), + patch.object(pc, "record_surfaced", MagicMock()), + patch.object(pc, "owner_names_for", AsyncMock(return_value={})), + patch.object(pc.rulebooks_svc, "list_always_on_rules", + AsyncMock(return_value=rules)), + ) + + +async def _hint(rules, held_etag): + from scribe.services import plugin_context as pc + + patches = _quiet_write_path(pc, rules) + for p in patches: + p.start() + try: + out = await pc.build_write_path_hint( + 1, "src/scribe/services/rulebooks.py", code="x" * 400, + rules_etag=held_etag, + ) + finally: + for p in patches: + p.stop() + return out["context"] + + +@pytest.mark.asyncio +async def test_a_session_is_told_which_rule_moved(): + """The delivery, end to end through the real hint builder. Naming the rule + is the point — "something changed" sends the reader to re-read everything, + which is the cost the marker was meant to avoid.""" + held = svc.rules_etag([_rule(rid=1), _rule(rid=2, title="dev is home")]) + current = [_rule(rid=1), _rule(rid=2, title="dev is home", updated_at=LATER)] + + ctx = await _hint(current, held) + assert "changed since this session started" in ctx + assert "#2" in ctx and "dev is home" in ctx + + +@pytest.mark.asyncio +async def test_a_session_holding_the_current_rules_is_told_nothing(): + """The one that keeps the signal worth reading. A line on every write is a + line nobody reads.""" + rules = [_rule(rid=1), _rule(rid=2)] + ctx = await _hint(rules, svc.rules_etag(rules)) + assert "changed since this session started" not in ctx + + +@pytest.mark.asyncio +async def test_a_session_that_sent_no_marker_is_told_nothing(): + """An install whose hook never reached /api/plugin/context has nothing + stored. Absent must read as silence, not as a mismatch — otherwise the + first thing a new install hears is that its rules changed.""" + ctx = await _hint([_rule(updated_at=LATER)], "") + assert "changed since this session started" not in ctx + + +@pytest.mark.asyncio +async def test_a_DELETED_rule_is_reported_even_though_it_has_no_row(): + """The count arm. A deleted rule leaves nothing to name, and it is the + change that takes an instruction OUT of force — so "no longer in force" + has to be sayable without a row to say it about.""" + held = svc.rules_etag([_rule(rid=1), _rule(rid=2)]) + ctx = await _hint([_rule(rid=1)], held) + assert "no longer in force" in ctx + + +@pytest.mark.asyncio +async def test_the_arm_fails_open(): + """A staleness hint must never break a write. Every other arm here fails + open for the same reason, and this one runs a query that can fail.""" + from scribe.services import plugin_context as pc + + patches = _quiet_write_path(pc, []) + for p in patches: + p.start() + try: + with patch.object(pc.rulebooks_svc, "list_always_on_rules", + AsyncMock(side_effect=RuntimeError("database down"))): + out = await pc.build_write_path_hint( + 1, "src/x.py", code="x" * 400, rules_etag="2026-01-01T00:00:00+00:00|3", + ) + finally: + for p in patches: + p.stop() + assert "changed since this session started" not in out["context"] + + +def test_the_marker_cannot_break_the_payload_it_decorates(): + """It is computed on the SessionStart path. Raising there would cost the + whole context payload — every rule title, the project, the lot — to save + a hint, which is the wrong trade in every case. + + A row with no usable timestamp is skipped; a set with none degrades to a + count-only marker. Count-only still catches a rule ADDED or DELETED and + only loses edits, which is the right way round to lose information. + + Found by CI: `build_session_context` tests hand it MagicMock rules, and + `max()` over those raises TypeError rather than returning anything. + """ + from unittest.mock import MagicMock + + assert svc.rules_etag([MagicMock(), MagicMock()]) == "unknown|2" + assert svc.rules_etag([SimpleNamespace()]) == "unknown|1" + # A count-only marker still moves when the set does. + assert svc.rules_etag([MagicMock()]) != svc.rules_etag([MagicMock(), MagicMock()]) + # One usable stamp is enough to keep the real thing. + assert svc.rules_etag([_rule(), MagicMock()]).startswith("2026-") + + +@pytest.mark.asyncio +async def test_the_signal_arrives_even_when_nothing_else_matched(): + """THE BUG CI CAUGHT, and the one the task's acceptance criterion was + written to catch. + + `build_write_path_hint` returns early when no prior art, stamp, divergence + or derive matched — which sat ABOVE this arm, so a session whose rules had + changed was told only if the file it happened to be editing also matched + something else. A staleness signal that fires on that coincidence is not a + staleness signal. + """ + held = svc.rules_etag([_rule(rid=1), _rule(rid=2, title="dev is home")]) + current = [_rule(rid=1), _rule(rid=2, title="dev is home", updated_at=LATER)] + + # Every other arm silent — which is exactly the case that used to return "". + ctx = await _hint(current, held) + assert "changed since this session started" in ctx diff --git a/tests/test_services_access_visibility.py b/tests/test_services_access_visibility.py index e036fad..421e528 100644 --- a/tests/test_services_access_visibility.py +++ b/tests/test_services_access_visibility.py @@ -20,10 +20,9 @@ from scribe.services.access import ( notes_visibility_clause, readable_notes_clause, ) +from tests.helpers import compiled_sql - -def _sql(clause) -> str: - return str(clause.compile(compile_kwargs={"literal_binds": True})) +_sql = compiled_sql def _read(user_id: int = 7) -> str: diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index a5d78bc..274fde3 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -16,12 +16,246 @@ import pytest from scribe.services import backup -def test_backup_version_is_v8(): - """v7 added code_shapes (#2787), v8 its history (#2793). The bump is the - point of the test — a payload section added without moving the version - produces backups that are structurally different and indistinguishable - by inspection.""" - assert backup.BACKUP_VERSION == 10 +def test_backup_version_is_current(): + """The bump is the point of the test — a payload section added without + moving the version produces backups that are structurally different and + indistinguishable by inspection. + + (Named for the number it asserted until v10, which is exactly the drift a + name-carrying-a-value invites; it now says what it checks.)""" + assert backup.BACKUP_VERSION == 13 + + +def _exportable_note(**over): + """A Note-shaped stand-in for the pure row helper. SimpleNamespace, not a + MagicMock: `_note_rows` calls .isoformat() on the timestamps, and a mock + would happily return another mock instead of failing.""" + base = dict( + id=1, user_id=7, title="t", body="b", description=None, tags=["x"], + parent_id=None, arose_from_id=None, + project_id=None, milestone_id=None, status=None, priority=None, + due_date=None, created_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + updated_at=datetime(2026, 1, 2, tzinfo=timezone.utc), + note_type="note", task_kind="work", data=None, + started_at=None, completed_at=None, + recurrence_rule=None, recurrence_next_spawn_at=None, + verify_with=None, expires_when=None, verified_at=None, + ) + base.update(over) + return SimpleNamespace(**base) + + +def test_note_rows_carry_the_verification_trio(): + """Operator judgment — somebody checked this fact, and this is when — + which nothing downstream can recompute (milestone 317).""" + [row] = backup._note_rows([_exportable_note( + verify_with="curl the AMO docs", + expires_when="AMO starts allowing re-signing", + verified_at=datetime(2026, 8, 28, tzinfo=timezone.utc), + )]) + assert row["verify_with"] == "curl the AMO docs" + assert row["expires_when"] == "AMO starts allowing re-signing" + assert row["verified_at"] == "2026-08-28T00:00:00+00:00" + + +def test_a_never_checked_note_exports_a_null_stamp_and_restores_as_one(): + """The round trip that matters. NULL `verified_at` means nobody has ever + looked, and it is what sorts FIRST in the sweep. Restoring it as now() — + which is what `_dt` would do — silently converts the sweep's top result + into its bottom one.""" + [row] = backup._note_rows([_exportable_note(verify_with="check the runner shell")]) + assert row["verified_at"] is None + assert backup._dt_or_none(row["verified_at"]) is None + # ...and the helper that must NOT be used here, for contrast. + assert backup._dt(row["verified_at"]) is not None + + +def test_the_record_type_and_kind_survive_the_export(): + """#3182's headline. Without these two columns a restore reported success + and handed back a corpus where all 90 snippets and 3 processes were plain + notes and all 435 issues and the spike were `work` — the entire vocabulary + milestone 312 and #3128 were about, gone, with nothing to notice it by.""" + [snippet] = backup._note_rows([_exportable_note(note_type="snippet")]) + [issue] = backup._note_rows([_exportable_note(task_kind="issue", status="done")]) + assert snippet["note_type"] == "snippet" + assert issue["task_kind"] == "issue" + + +def test_provenance_and_lifecycle_travel(): + started = datetime(2026, 3, 1, tzinfo=timezone.utc) + [row] = backup._note_rows([_exportable_note( + arose_from_id=42, + description="one-liner", + started_at=started, + recurrence_rule={"freq": "weekly"}, + )]) + assert row["arose_from_id"] == 42 + assert row["description"] == "one-liner" + assert row["started_at"] == started.isoformat() + assert row["recurrence_rule"] == {"freq": "weekly"} + # Absent lifecycle stamps stay absent — a note that never started must not + # restore as one that started at restore time. + assert row["completed_at"] is None + + +def test_a_milestone_carries_its_plan(): + """A milestone IS the plan (0066); `body` is its design and intent and + `description` is only the one-line summary. Dropping it restored every + plan as a title with no reasoning behind it (#3182).""" + m = SimpleNamespace( + id=1, user_id=7, project_id=2, title="t", description="d", + body="## Goal\n\nthe actual plan", status="active", order_index=0, + created_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + updated_at=datetime(2026, 1, 2, tzinfo=timezone.utc), + ) + [row] = backup._milestone_rows([m]) + assert row["body"] == "## Goal\n\nthe actual plan" + + +def test_a_repo_binding_carries_the_branch_its_ledger_follows(): + """#2873. Without `ref` a restored binding silently falls back to the + default branch and the shape ledger starts accounting for a different + tree — a wrong answer that looks like a working one.""" + b = SimpleNamespace(user_id=7, project_id=2, repo_key="Scribe", ref="dev") + [row] = backup._repo_binding_rows([b]) + assert row["ref"] == "dev" + + +# The table -> (model, row helper) registry the column guard walks. Kept here +# rather than in the service because it exists only to be introspected: the +# product code already knows these pairings by calling them. +def _column_guard_targets(): + from scribe.models.canonical_system import CanonicalSystem + from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse + from scribe.models.design_system import DesignSystem, DesignToken + from scribe.models.milestone import Milestone + from scribe.models.note import Note + from scribe.models.note_draft import NoteDraft + from scribe.models.note_supersession import NoteSupersession + from scribe.models.note_usage import NoteUsageEvent + from scribe.models.note_version import NoteVersion + from scribe.models.rule_version import RuleVersion + from scribe.models.project import Project + from scribe.models.repo_binding import RepoBinding + from scribe.models.rulebook import Rule, Rulebook, RulebookTopic, RuleRelation + from scribe.models.setting import Setting + from scribe.models.system import RecordSystem, System + from scribe.models.task_log import TaskLog + from scribe.models.user import User + + return { + "users": (User, backup._user_rows), + "projects": (Project, backup._project_rows), + "milestones": (Milestone, backup._milestone_rows), + "notes": (Note, backup._note_rows), + "task_logs": (TaskLog, backup._task_log_rows), + "note_drafts": (NoteDraft, backup._note_draft_rows), + "note_versions": (NoteVersion, backup._note_version_rows), + "settings": (Setting, backup._setting_rows), + "rulebooks": (Rulebook, backup._rulebook_rows), + "rulebook_topics": (RulebookTopic, backup._topic_rows), + "rules": (Rule, backup._rule_rows), + "rule_versions": (RuleVersion, backup._rule_version_rows), + "systems": (System, lambda rows: backup._system_rows(rows, {})), + "canonical_systems": (CanonicalSystem, backup._canonical_system_rows), + "record_systems": (RecordSystem, backup._record_system_rows), + "note_supersessions": (NoteSupersession, backup._note_supersession_rows), + "rule_relations": (RuleRelation, backup._rule_relation_rows), + "note_usage_events": (NoteUsageEvent, backup._usage_event_rows), + "design_systems": (DesignSystem, backup._design_system_rows), + "design_tokens": (DesignToken, backup._design_token_rows), + "repo_bindings": (RepoBinding, backup._repo_binding_rows), + "code_shapes": (CodeShape, backup._code_shape_rows), + "code_shape_events": (CodeShapeEvent, backup._code_shape_event_rows), + "code_shape_uses": (CodeShapeUse, backup._code_shape_use_rows), + } + + +def _stand_in(model): + """A real instance of `model` with every column set to a value of roughly + the right type, so the serialiser runs and we can read which KEYS it + produced. Values are meaningless; only the shape of the output dict is + under test. + + A real instance rather than a MagicMock because several helpers delegate to + the model's own `to_dict()`, and a mock would return another mock instead + of a dict. Typed rather than a bare instance because the helpers call + `.isoformat()` on the timestamps, which `None` does not have. + """ + import sqlalchemy as sa + + row = model() + for column in model.__table__.columns: + t = column.type + if isinstance(t, sa.DateTime): + value = datetime(2026, 1, 1, tzinfo=timezone.utc) + elif isinstance(t, sa.Date): + value = datetime(2026, 1, 1).date() + elif isinstance(t, sa.Boolean): + value = False + elif isinstance(t, sa.Integer): + value = 1 + elif isinstance(t, sa.ARRAY) or isinstance(getattr(t, "impl", None), sa.ARRAY): + value = [] + elif isinstance(t, (sa.Text, sa.String)): + value = "x" + else: + # JSON/JSONB and anything exotic. None is what these actually hold + # most of the time, and no serialiser calls a method on one. + value = None + setattr(row, column.name, value) + return row + + +@pytest.mark.parametrize("table", sorted(_column_guard_targets())) +def test_every_column_is_exported_or_declared_excluded(table): + """THE COLUMN GUARD (#3182) — _NOT_INCLUDED's shape, one level down. + + The table guard below catches a whole table going missing. It cannot catch + a COLUMN going missing from a table it already considers covered, which is + how nine of them vanished from `notes` alone: note_type and task_kind, so + every snippet and process restored as a plain note and every issue and + spike as `work`; arose_from_id, so every provenance edge went; the + recurrence pair, so recurring tasks stopped recurring. Plus milestones.body + — which IS the plan — and repo_bindings.ref. + + Each arrived the same way: added to the model and the migration, both of + which fail loudly, and never to the serialiser, which fails silently. + + A new column must now be exported or named in _COLUMN_EXCLUSIONS with a + reason. Forgetting is no longer expressible. + """ + model, helper = _column_guard_targets()[table] + [row] = helper([_stand_in(model)]) + + columns = {c.name for c in model.__table__.columns} + missing = columns - set(row) + declared = backup._COLUMN_EXCLUSIONS[table] + + assert missing == declared, ( + f"{table}: exported columns and _COLUMN_EXCLUSIONS disagree.\n" + f" dropped but not declared: {sorted(missing - declared)}\n" + f" declared but exported anyway: {sorted(declared - missing)}" + ) + + +def test_the_column_guard_covers_every_table_with_a_row_helper(): + """The guard is only as good as its registry — a table added to _BACKED_UP + with a new helper, and not to the registry, would be unguarded and look + guarded. Join tables have no model class and carry both their columns by + construction, so they are the only permitted absences.""" + # REAL table names, as _BACKED_UP holds them — not the shorter keys the + # payload uses for the same sections. Getting this wrong is what the guard + # caught on its own first run. + join_tables = { + "project_rulebook_subscriptions", "project_rule_suppressions", + "project_topic_suppressions", "project_rulebook_exclusions", + "rule_systems", + } + covered = set(_column_guard_targets()) | join_tables + assert set(backup._BACKED_UP) - covered == set() + # And no stale entries: every declaration must name a real target. + assert set(backup._COLUMN_EXCLUSIONS) == set(_column_guard_targets()) def test_not_included_lists_the_known_gaps(): diff --git a/tests/test_services_knowledge_counts.py b/tests/test_services_knowledge_counts.py index 497835e..efe6516 100644 --- a/tests/test_services_knowledge_counts.py +++ b/tests/test_services_knowledge_counts.py @@ -1,4 +1,12 @@ -"""get_knowledge_counts includes the 'process' type and counts it in total.""" +"""get_knowledge_counts — one number per facet, and an honest total. + +Two grouped queries, one per typing axis. It used to be three: a grouped count +over note_type restricted to ("note", "process"), a scalar count of tasks, and +a second scalar just for plans. That shape is why `issue` had no number — +every kind needed a query of its own and nobody added one — and why the "All" +chip sat ~90 below the list it labelled, since snippets were in the feed but +in no count (#3128). +""" from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -11,29 +19,68 @@ def _grouped(rows): return r -def _scalar(n): - r = MagicMock() - r.scalar_one.return_value = n - return r - - -@pytest.mark.asyncio -async def test_counts_include_process_in_facet_and_total(): +async def _counts(non_task_rows, kind_rows, **kwargs): session = make_mock_session() - # 1) grouped non-task counts, 2) task count, 3) plan count - session.execute = AsyncMock(side_effect=[ - _grouped([("note", 3), ("process", 2)]), - _scalar(1), # tasks - _scalar(0), # plans - ]) + # 1) non-task rows grouped by note_type, 2) task rows grouped by task_kind + session.execute = AsyncMock( + side_effect=[_grouped(non_task_rows), _grouped(kind_rows)] + ) with patch("scribe.services.knowledge.async_session") as cls: cls.return_value = session from scribe.services.knowledge import get_knowledge_counts - counts = await get_knowledge_counts(user_id=1) + return await get_knowledge_counts(user_id=1, **kwargs), session - assert counts["process"] == 2 - # facet keys all present (setdefault) - for key in ("note", "task", "plan", "process"): - assert key in counts - # total = note(3) + task(1) + process(2) - assert counts["total"] == 6 + +@pytest.mark.asyncio +async def test_every_facet_gets_a_number_including_the_kinds(): + counts, _ = await _counts( + [("note", 395), ("process", 3), ("snippet", 90)], + [("work", 2104), ("issue", 435), ("spike", 1), ("plan", 90)], + ) + assert counts["note"] == 395 + assert counts["process"] == 3 + assert counts["snippet"] == 90 + assert counts["issue"] == 435 + assert counts["spike"] == 1 + assert counts["plan"] == 90 + assert counts["work"] == 2104 + + +@pytest.mark.asyncio +async def test_task_is_the_sum_of_its_kinds(): + """`task` is not counted separately any more — it is what the kinds add up + to, so the two can't disagree.""" + counts, _ = await _counts([], [("work", 2104), ("issue", 435), ("spike", 1)]) + assert counts["task"] == 2540 + + +@pytest.mark.asyncio +async def test_total_counts_snippets_and_counts_no_task_twice(): + """The All chip labels a feed that contains every kind, so it has to count + every kind — and exactly once. Kinds are subsets of `task`; adding them + would count each issue a second time.""" + counts, _ = await _counts( + [("note", 10), ("process", 2), ("snippet", 5)], + [("work", 20), ("issue", 4)], + ) + assert counts["task"] == 24 + assert counts["total"] == 10 + 2 + 5 + 24 + + +@pytest.mark.asyncio +async def test_absent_facets_report_zero_rather_than_missing(): + counts, _ = await _counts([("note", 1)], []) + assert counts["note"] == 1 + for key in ("process", "snippet", "task", "work", "issue", "spike", "plan"): + assert counts[key] == 0, key + assert counts["total"] == 1 + + +@pytest.mark.asyncio +async def test_a_tag_filter_narrows_both_axes(): + """A tag has to reach both queries, or the chips would disagree with each + other under a filter — tasks narrowed, notes not.""" + _, session = await _counts([], [], tags=["python"]) + assert session.execute.await_count == 2 + for call in session.execute.await_args_list: + assert "notes.tags" in str(call.args[0]) diff --git a/tests/test_services_notes_sweep.py b/tests/test_services_notes_sweep.py new file mode 100644 index 0000000..a611b78 --- /dev/null +++ b/tests/test_services_notes_sweep.py @@ -0,0 +1,242 @@ +"""The note staleness sweep (milestone 317 step 3). + +The query is a sibling of the rules sweep, not a shared implementation — a +rule scopes by rulebook ownership, a note by the note ACL, so only the +BEHAVIOUR is common (services/verification). These pin the parts that would +fail silently rather than loudly: the ordering, which rows are eligible, and +the asymmetry of a failed check. +""" +from datetime import datetime, timedelta, timezone +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from tests.helpers import compiled_sql, fake_note, make_mock_session + + +async def _sweep_sql(**kwargs): + """The statement the sweep builds, as SQL text. + + No database: the SHAPE of the query is what is under test, and the + ordering clause in particular cannot be checked any other way without one. + The await happens INSIDE the patch — a coroutine created in the block and + awaited outside it would run against the real session. + """ + captured = {} + session = make_mock_session() + + async def _execute(stmt): + try: + captured["sql"] = compiled_sql(stmt) + except Exception: + # Literal-rendering a datetime bind is dialect-dependent and can + # raise. Only the tests asserting on a literal value (user_id, + # project_id) need that form, and none of those build a cutoff. + captured["sql"] = str(stmt) + result = MagicMock() + result.scalars.return_value.all.return_value = [] + return result + + session.execute = _execute + with patch("scribe.services.notes.async_session") as cls: + cls.return_value = session + from scribe.services.notes import notes_due_for_verification + await notes_due_for_verification(user_id=7, **kwargs) + return captured["sql"] + + +def _where(sql: str) -> str: + """Just the WHERE clause. `select(Note)` names every column, so searching + the whole statement for a column name always finds the SELECT list — which + is how one of these tests first failed for the wrong reason. + + Matched by regex rather than split on a literal, because the exact + whitespace SQLAlchemy emits around WHERE is not something a test should + depend on. + """ + import re + + m = re.search(r"\bWHERE\b(.*?)(?:\bORDER BY\b|$)", sql, re.S) + assert m, "no WHERE clause — the sweep must never select the whole table" + return m.group(1) + + +# ── the ordering, which is the whole signal ────────────────────────────────── + +@pytest.mark.asyncio +async def test_never_checked_sorts_first_not_last(): + """THE thing most likely to be got wrong, and it would not error. + + Postgres sorts NULLs LAST on ASC by default, so the obvious `ORDER BY + verified_at ASC` sinks every never-checked note below every checked one — + exactly inverting the signal the sweep exists to carry. A note nobody has + ever confirmed is a claim with no evidence behind it at all. + """ + sql = await _sweep_sql() + assert "ORDER BY notes.verified_at ASC NULLS FIRST" in sql + + +@pytest.mark.asyncio +async def test_the_order_is_total(): + """A tiebreak, so two notes verified in the same transaction do not swap + places between calls and make a page boundary lie. + + Asserted on the END of the statement, not by searching it: `select(Note)` + names every column, so the first `notes.id` in the text is the SELECT + list, not the ORDER BY.""" + sql = (await _sweep_sql()).rstrip() + assert sql.endswith("ORDER BY notes.verified_at ASC NULLS FIRST, notes.id") + + +# ── which rows are eligible ────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_only_notes_that_carry_a_check_appear(): + """A note with no `verify_with` is not overdue — it is a decision, and + listing it would dilute the result until nobody reads it.""" + assert "notes.verify_with IS NOT NULL" in await _sweep_sql() + + +@pytest.mark.asyncio +async def test_trashed_notes_are_excluded(): + assert "notes.deleted_at IS NULL" in await _sweep_sql() + + +@pytest.mark.asyncio +async def test_the_scope_is_browse_not_read(): + """Decision note 2094: a sweep is a PASSIVE surface, so a record shared + one-to-one must not arrive in one unasked. `note_shares` is the tell — + its presence would mean the read scope leaked in.""" + sql = await _sweep_sql() + assert "notes.user_id = 7" in sql + assert "project_shares" in sql + assert "note_shares" not in sql + + +@pytest.mark.asyncio +async def test_a_task_carrying_a_check_is_NOT_filtered_out(): + """Deliberate. The write path permits a check on nothing but a plain note, + so such a row would be in an ILLEGAL state — and this is the one surface + that could tell somebody. Hiding it to match the invariant would make the + sweep agree with a database it had stopped describing.""" + where = _where(await _sweep_sql()) + assert "notes.status" not in where + assert "notes.note_type" not in where + + +# ── the filters ────────────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_never_only_narrows_to_the_unexamined(): + assert "notes.verified_at IS NULL" in await _sweep_sql(never_only=True) + + +@pytest.mark.asyncio +async def test_an_age_window_still_includes_the_never_checked(): + """They are the most overdue thing there is; a window that excluded them + would answer the opposite of the question.""" + sql = await _sweep_sql(older_than_days=30) + assert "notes.verified_at IS NULL" in sql + assert "notes.verified_at <" in sql + + +@pytest.mark.asyncio +async def test_never_only_wins_over_an_age_window(): + """Both together is a caller contradicting themselves; the narrower one is + the safe reading, and it must not emit a cutoff as well.""" + sql = await _sweep_sql(never_only=True, older_than_days=30) + assert "notes.verified_at <" not in sql + + +@pytest.mark.asyncio +async def test_a_project_filter_narrows(): + assert "notes.project_id = 4" in await _sweep_sql(project_id=4) + + +@pytest.mark.asyncio +async def test_a_negative_window_raises_rather_than_meaning_everything(): + """Silently answering a different question is the failure this guards.""" + with pytest.raises(ValueError, match="older_than_days"): + await _sweep_sql(older_than_days=-1) + + +# ── the stamp ──────────────────────────────────────────────────────────────── + +async def _mark(note, still_true=True, writable=True): + session = make_mock_session() + result = MagicMock() + result.scalars.return_value.first.return_value = note + session.execute = AsyncMock(return_value=result) + with patch("scribe.services.notes.async_session") as cls, \ + patch("scribe.services.access.can_write_note", + AsyncMock(return_value=writable)): + cls.return_value = session + from scribe.services.notes import mark_note_verified + return await mark_note_verified(note_id=1, user_id=7, + still_true=still_true), session + + +@pytest.mark.asyncio +async def test_a_passing_check_stamps_the_note(): + note = fake_note(verify_with="curl the docs", verified_at=None) + out, session = await _mark(note) + assert out is note + assert note.verified_at is not None + session.commit.assert_awaited() + + +@pytest.mark.asyncio +async def test_a_failing_check_writes_nothing(): + """The asymmetry IS the design. There is no "verified false" state, + because a note whose check failed is not in a special condition — it is + WRONG. Recording the failure as a flag would let it sit there being false + with the sweep quietly satisfied that somebody had looked.""" + note = fake_note(verify_with="curl the docs", verified_at=None) + out, session = await _mark(note, still_true=False) + assert out is note + assert note.verified_at is None, "a failed check must leave the stamp alone" + session.commit.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_a_note_with_no_check_cannot_be_verified(): + """Nothing to verify is a different answer from verified.""" + note = fake_note(verify_with=None) + out, _ = await _mark(note) + assert out is None + + +@pytest.mark.asyncio +async def test_a_reader_cannot_stamp(): + """Stamping is a mutation (rules 47/78) — an editor-share holder may make + it, a viewer may not.""" + note = fake_note(verify_with="curl the docs", verified_at=None) + out, _ = await _mark(note, writable=False) + assert out is None + assert note.verified_at is None + + +# ── the row ────────────────────────────────────────────────────────────────── + +def test_the_row_carries_the_check_in_full(): + """The opposite call from a listing: the caller is about to go and run it, + so the text IS the payload rather than the bloat.""" + note = fake_note( + id=9, title="Versioning", project_id=2, + verify_with="curl the AMO docs", expires_when="AMO allows re-signing", + verified_at=datetime.now(timezone.utc) - timedelta(days=74), + ) + from scribe.services.notes import verification_row + row = verification_row(note) + assert row["verify_with"] == "curl the AMO docs" + assert row["expires_when"] == "AMO allows re-signing" + assert row["days_since_verified"] == 74 + + +def test_a_never_checked_row_says_never_not_none(): + """None means "this is a decision, the question does not apply"; "never" + means "it asserts a fact and nobody has confirmed it". The second is the + one worth acting on, and collapsing them loses the sweep's point.""" + from scribe.services.notes import verification_row + row = verification_row(fake_note(verify_with="a check", verified_at=None)) + assert row["last_verified"] == "never" + assert row["days_since_verified"] is None diff --git a/tests/test_services_notes_verification.py b/tests/test_services_notes_verification.py new file mode 100644 index 0000000..8b7debd --- /dev/null +++ b/tests/test_services_notes_verification.py @@ -0,0 +1,172 @@ +"""A note's own check — verify_with / expires_when (milestone 317 step 2). + +Three behaviours the rules path (#3096) had to get right and this one inherits: +empty means NULL, clearing is explicit, and rewriting the check drops the +stamp. Plus one this path adds: not every record may carry a check, and the +rule is an INVARIANT over the record rather than a filter on the write. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from tests.helpers import drive_update_note as _update +from tests.helpers import fake_note, make_mock_session + + +def _checkable(**over): + """A plain note — no status, no snippet type, no check. `fake_note` is a + MagicMock, so every one of these must be set explicitly: an unset + attribute is a truthy mock, which would look like a check that is there.""" + base = dict( + status=None, note_type="note", + verify_with=None, expires_when=None, verified_at=None, + project_id=None, + ) + base.update(over) + return fake_note(**base) + + +async def _create(**kwargs): + session = make_mock_session() + captured = {} + session.add = MagicMock(side_effect=lambda obj: captured.update( + verify_with=getattr(obj, "verify_with", "MISSING"), + expires_when=getattr(obj, "expires_when", "MISSING"), + )) + with patch("scribe.services.notes.async_session") as cls, \ + patch("scribe.services.notes.embed_note", MagicMock()), \ + patch("scribe.services.notes._maybe_reactivate_project", AsyncMock()): + cls.return_value = session + from scribe.services.notes import create_note + await create_note(user_id=1, title="t", **kwargs) + return captured + + +# ── empty means empty ──────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_an_empty_check_is_stored_as_null_not_as_a_blank(): + """The sweep's whole signal is `verify_with IS NULL` = "this is a decision, + there is nothing to check". A "" that is not NULL makes a norm look like a + constraint nobody has verified — and never-checked sorts FIRST, so it would + sit at the top of the sweep forever.""" + assert (await _create(verify_with="", expires_when=""))["verify_with"] is None + + note = _checkable(verify_with="curl the docs") + await _update(note, verify_with="") + assert note.verify_with is None + + +@pytest.mark.asyncio +async def test_a_check_is_stored_when_given(): + captured = await _create(verify_with="curl the AMO docs", expires_when="AMO allows re-signing") + assert captured["verify_with"] == "curl the AMO docs" + assert captured["expires_when"] == "AMO allows re-signing" + + +# ── clearing is explicit ───────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_clear_unsets_a_field_the_mcp_door_cannot_empty(): + """At the MCP door "" means "leave this alone", so an agent updating a body + does not wipe a check it was never asked about. That leaves no value + meaning "remove it" — hence naming the field, which cannot happen by + accident.""" + note = _checkable(verify_with="a check", expires_when="a state") + await _update(note, clear=["verify_with"]) + assert note.verify_with is None + assert note.expires_when == "a state", "clearing one must not clear the other" + + +@pytest.mark.asyncio +async def test_clear_ignores_a_field_that_is_not_clearable(): + note = _checkable(title="keep me", verify_with="a check") + await _update(note, clear=["title"]) + assert note.title == "keep me" + + +# ── the stamp certifies a check, not a record ──────────────────────────────── + +@pytest.mark.asyncio +async def test_rewriting_the_check_drops_the_stamp(): + note = _checkable(verify_with="the old check", verified_at="2026-01-01") + await _update(note, verify_with="a different check") + assert note.verified_at is None + + +@pytest.mark.asyncio +async def test_clearing_the_check_drops_the_stamp(): + note = _checkable(verify_with="the old check", verified_at="2026-01-01") + await _update(note, clear=["verify_with"]) + assert note.verified_at is None + + +@pytest.mark.asyncio +async def test_an_unchanged_check_keeps_its_stamp(): + """Only a CHANGE invalidates it — otherwise every unrelated edit would + re-enter the note into the sweep and the signal would mean nothing.""" + note = _checkable(verify_with="the same check", verified_at="2026-01-01") + await _update(note, title="a new title", verify_with="the same check") + assert note.verified_at == "2026-01-01" + + +@pytest.mark.asyncio +async def test_a_stamp_cannot_be_set_through_an_ordinary_edit(): + """A stamp says somebody performed THIS check. Minting one from a write + that ran no check is the one thing that would make the sweep lie.""" + note = _checkable(verify_with="a check", verified_at=None) + await _update(note, verified_at="2026-08-28") + assert note.verified_at is None + + +# ── the invariant: which records may carry a check ─────────────────────────── + +@pytest.mark.asyncio +async def test_a_task_is_refused_a_check(): + with pytest.raises(ValueError, match="task"): + await _create(status="todo", verify_with="a check") + + +@pytest.mark.asyncio +async def test_a_snippet_is_refused_and_told_where_to_go(): + """The message has to name the alternative, or the caller is left with a + refusal and no route.""" + with pytest.raises(ValueError, match="verify_snippet"): + await _create(note_type="snippet", verify_with="a check") + + +@pytest.mark.asyncio +async def test_turning_a_checked_note_into_a_task_is_refused(): + """THE reason this is an invariant over the resulting record and not a + filter on which fields were passed. This write names no check at all, and + would sail past any per-field gate.""" + note = _checkable(verify_with="a check") + with pytest.raises(ValueError, match="task"): + await _update(note, status="todo") + + +@pytest.mark.asyncio +async def test_an_unchecked_note_can_still_become_a_task(): + """The invariant must not make ordinary promotion impossible.""" + note = _checkable() + await _update(note, status="todo") + assert note.status == "todo" + + +@pytest.mark.asyncio +async def test_clearing_the_check_in_the_same_write_lets_it_become_a_task(): + """The error tells the caller to clear the check first; doing both at once + has to actually work, or the advice is wrong.""" + note = _checkable(verify_with="a check") + await _update(note, status="todo", clear=["verify_with"]) + assert note.status == "todo" + assert note.verify_with is None + + +@pytest.mark.asyncio +async def test_an_ordinary_note_write_is_untouched_by_any_of_this(): + """The common case: no check, nothing to guard, nothing to reset.""" + note = _checkable(title="before") + await _update(note, title="after") + assert note.title == "after" + assert note.verify_with is None + assert note.verified_at is None diff --git a/tests/test_snippet_mirror_generic_door.py b/tests/test_snippet_mirror_generic_door.py new file mode 100644 index 0000000..30af5e2 --- /dev/null +++ b/tests/test_snippet_mirror_generic_door.py @@ -0,0 +1,95 @@ +"""A snippet's `data` mirror survives the GENERIC note door. + +`notes.data` is derived from the body. The snippet service always composed it +from the field set it had just merged, so `update_snippet` was never the +problem — the problem was every other way a snippet's body could be written. +`update_note` is a `hasattr` loop with no snippet awareness, and both doors +reach it: PATCH /api/notes/ and the MCP update_note tool. The Knowledge +feed handed you that path, because a snippet card there routed to /notes/:id. + +The failure was silent and the wrong way round: `snippet_fields` PREFERS the +mirror, so the row went on reporting its old repo/path/symbol to the location +reverse lookup and to prior-art recall while displaying its new body — a record +surfaced with full authority and wrong, which the drift-check docstring calls +worse than having no record at all (#3128). +""" +import pytest +from tests.helpers import drive_update_note as _update +from tests.helpers import fake_note, fake_snippet + +OLD_MIRROR = { + "name": "debounce", + "language": "javascript", + "locations": [{"repo": "Scribe", "path": "old/place.js", "symbol": "debounce"}], + "verification": {"status": "ok", "code_sha": "abc", "checked_at": "2026-01-01"}, + "provenance": {"commit_sha": "deadbeef"}, +} + +MOVED_BODY = ( + "**Locations:**\n" + "- `Scribe` · `new/place.ts` · `debounce`\n\n" + "```typescript\nexport const debounce = 1;\n```\n" +) + + +@pytest.mark.asyncio +async def test_a_body_write_moves_the_mirror_with_it(): + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, body=MOVED_BODY) + assert note.data["locations"] == [ + {"repo": "Scribe", "path": "new/place.ts", "symbol": "debounce"} + ], "the mirror still describes where the snippet used to live" + assert note.data["language"] == "typescript" + + +@pytest.mark.asyncio +async def test_the_verdict_and_provenance_are_carried_not_dropped(): + """Neither is in the body to parse, so recomposing must carry them. An + ordinary edit must not erase the last drift check — and it needs no + invalidation branch either: `code_sha` is recomputed from the new code, so + a verdict stamped against the old code expires itself on read.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, body=MOVED_BODY) + assert note.data["verification"] == OLD_MIRROR["verification"] + assert note.data["provenance"] == OLD_MIRROR["provenance"] + assert note.data["code_sha"] != OLD_MIRROR["verification"]["code_sha"] + + +@pytest.mark.asyncio +async def test_an_explicit_data_wins_over_recomposition(): + """`update_snippet` composes the mirror from the merged field set it holds + and passes it here. That caller knows things the body cannot be re-read for + — which locations were replaced, whether provenance survives the edit — so + an explicit mirror must not be recomputed out from under it.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + authoritative = {"name": "from the service", "locations": []} + await _update(note, body=MOVED_BODY, data=authoritative) + assert note.data == authoritative + + +@pytest.mark.asyncio +async def test_a_plain_note_is_left_alone(): + """Only snippets carry a mirror; a note's `data` must not be invented.""" + note = fake_note(note_type="note", data=None, project_id=None) + await _update(note, body="just some prose") + assert note.data is None + + +@pytest.mark.asyncio +async def test_a_write_that_cannot_change_the_parse_does_not_touch_the_mirror(): + """Status, priority, project — none of them is an input to the body parser, + so recomposing on them would be work for nothing and would rebuild a mirror + from a body nobody claimed to have changed.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, project_id=4) + assert note.data == OLD_MIRROR + + +@pytest.mark.asyncio +async def test_a_title_change_reaches_the_mirror_too(): + """A snippet's NAME lives in its title, not its body — `parse_snippet_fields` + reads both, so both are triggers.""" + note = fake_snippet(data=dict(OLD_MIRROR), project_id=None) + await _update(note, title="throttle — cap a callback's rate") + assert note.data["name"] == "throttle" + assert note.data["when_to_use"] == "cap a callback's rate" diff --git a/tests/test_verification_guidance_survives.py b/tests/test_verification_guidance_survives.py new file mode 100644 index 0000000..aa7fc45 --- /dev/null +++ b/tests/test_verification_guidance_survives.py @@ -0,0 +1,108 @@ +"""The write surfaces say WHEN a note earns a check — not just that it can. + +WHY THIS EXISTS + +`verify_with` is a free-text field on the highest-volume record kind in the +product. A field described only as "how to verify this note" gets filled in on +every note within a week, and at that point the sweep returns the whole corpus +and means nothing. The signal is not "has a check" — it is "has a check AND +almost nothing else does". + +So the guidance is not decoration on this feature, it IS the feature's +precondition, and rule 119 puts it in the tool docstrings and the skill rather +than in a Scribe rule. That makes it exactly the kind of prose a later +docstring tidy-up deletes without noticing what it was for. + +WHAT THIS PINS, AND WHAT IT DOES NOT + +Structure, never wording, for `test_create_tools_disambiguate`'s reason: a test +that punishes rewriting is a test that gets deleted. Each write surface must +still (a) draw the norm-vs-constraint distinction in some form, (b) say the +empty case is normal, and (c) name where NOT to reach for it. + +It cannot tell whether the guidance is any good — only that the paragraph +explaining when to leave the field alone has not quietly become a parameter +list. +""" +import re + +import pytest +from tests.helpers import tool_doc as _doc + +# The surfaces that OFFER the field. The read surfaces (the sweep, the stamp) +# explain what a RESULT means, which is a different job — they are deliberately +# not held to this. +_WRITE_SURFACES = [ + ("scribe.mcp.tools.notes", "create_note"), + ("scribe.mcp.tools.notes", "update_note"), +] + + +@pytest.mark.parametrize("module,name", _WRITE_SURFACES) +def test_the_norm_versus_constraint_distinction_is_stated(module, name): + """The whole discipline in one line: a decision cannot go stale, a claim + about someone else's software can. Without it, "how would you check this" + reads as a chore to complete rather than a question with a usual answer of + "you wouldn't".""" + doc = _doc(module, name).lower() + assert "norm" in doc and "constraint" in doc, ( + f"{name} no longer draws the norm-vs-constraint distinction. Without " + f"it the field is just a box, and a box gets filled in." + ) + + +@pytest.mark.parametrize("module,name", _WRITE_SURFACES) +def test_the_empty_case_is_stated_as_normal(module, name): + """Said POSITIVELY, or an empty field reads as an unfinished record. This + is the single sentence standing between the sweep and irrelevance.""" + doc = _doc(module, name).lower() + assert re.search(r"leave (it|both|them|this|these)? ?empty|empty for", doc), ( + f"{name} no longer says that leaving the check empty is the normal " + f"case. Most notes are decisions; the field's default must read as a " + f"deliberate state, not a gap." + ) + + +@pytest.mark.parametrize("module,name", _WRITE_SURFACES) +def test_the_wrong_places_to_reach_for_it_are_named(module, name): + """A task's decay is its status; a snippet has verify_snippet. The service + refuses both — this is what should mean nobody ever hits that error.""" + doc = _doc(module, name).lower() + assert "task" in doc and "snippet" in doc, ( + f"{name} no longer names the records that must NOT carry a check. The " + f"gate still refuses them, but a refusal the caller could have " + f"foreseen is a worse door than one that explained itself." + ) + + +def test_expires_when_is_described_as_a_state_not_a_date(): + """The one field whose obvious reading is wrong. A date invents a staleness + schedule nobody can justify; a constraint expires when the ground moves, + which is a condition and not a time.""" + doc = _doc("scribe.mcp.tools.notes", "create_note") + assert "STATE" in doc, ( + "create_note no longer says expires_when is a STATE. Left to itself, " + '"expires" reads as a date, and every check would get an arbitrary one.' + ) + + +def test_the_skill_carries_the_test_a_writer_can_actually_apply(): + """The docstrings are read by whatever is holding the tool; the skill is + read while deciding what to write. The one-question form has to be in the + second place too, or the guidance only reaches callers who already opened + the tool.""" + import pathlib + + skill = pathlib.Path(__file__).resolve().parents[1] / ( + "plugin/skills/using-scribe/SKILL.md" + ) + text = " ".join(skill.read_text().split()) + assert "could this note become false without anyone editing it" in text.lower(), ( + "the using-scribe skill no longer carries the one-question test. That " + "question is what makes the distinction applicable rather than merely " + "true." + ) + assert "notes_due_for_verification" in text, ( + "the skill names the fields but not the surface that reads them — " + "guidance for writing a check with no route to acting on one." + )