diff --git a/backend/app/enrichment/describe.py b/backend/app/enrichment/describe.py index 8fcef9f..6b1d21d 100644 --- a/backend/app/enrichment/describe.py +++ b/backend/app/enrichment/describe.py @@ -27,6 +27,7 @@ from app.providers import build_provider from app.providers.base import ProviderError, ProviderUnavailable from app.services import assets as asset_service +from app.services import tags as tag_service from app.usage import events as usage_events logger = logging.getLogger(__name__) @@ -55,7 +56,7 @@ ) -def _prompt(asset: Asset, material: source.SourceMaterial) -> str: +def _prompt(asset: Asset, material: source.SourceMaterial, tag_names: list[str]) -> str: parts = [f"Filename: {asset.original_name or asset.name}"] if asset.summary: # Context, not something to restate — saying so is what stops the model @@ -64,6 +65,8 @@ def _prompt(asset: Asset, material: source.SourceMaterial) -> str: f"A summary already exists; do not repeat it, describe what is in the " f"item instead: {asset.summary}" ) + if tag_names: + parts.append("Tags already applied to this item: " + ", ".join(tag_names)) if material.kind == source.FROM_TRANSCRIPT: header = "Transcript of the recording" @@ -109,8 +112,9 @@ def run(session: Session, asset: Asset, progress: Progress) -> str: ) progress("Describing", 30, "") + tag_names = [t.name for t in tag_service.tags_for(session, asset.id)] completion = provider.complete( - _prompt(asset, material), + _prompt(asset, material, tag_names), system=SYSTEM_PROMPT, images=material.images, ) @@ -129,9 +133,7 @@ def run(session: Session, asset: Asset, progress: Progress) -> str: # land a description afterwards. progress("Saving", 90, "") - written = asset_service.apply_ai_metadata(session, asset, {"description": text}) - if not written: - return "Left alone — you wrote this description yourself" + asset_service.apply_ai_metadata(session, asset, {"description": text}) saw = " and a frame" if material.images and material.kind == source.FROM_TRANSCRIPT else "" logger.info( diff --git a/backend/app/enrichment/summarize.py b/backend/app/enrichment/summarize.py index 3cf099b..11faa90 100644 --- a/backend/app/enrichment/summarize.py +++ b/backend/app/enrichment/summarize.py @@ -1,8 +1,10 @@ """The summarize job: turn what an asset contains into a paragraph about it. The first job in M6 to produce something a person reads, and the first AI write path in -the app — so it is also the first caller of `apply_ai_metadata`, which is what keeps a -re-run from replacing a summary somebody wrote by hand (FR 8.1.3). +the app — so it is also the first caller of `apply_ai_metadata`. Pressing the button is +an explicit request for a fresh summary, so a re-run always replaces whatever is there, +including a summary somebody typed by hand — `apply_ai_metadata` still records that the +result came from AI, but nothing stops it from landing. What it reads is not this module's decision; `enrichment/source.py` makes it once for every job that will need it. For a transcribed video that is the transcript. @@ -20,6 +22,7 @@ from app.providers import build_provider from app.providers.base import ProviderError, ProviderUnavailable from app.services import assets as asset_service +from app.services import tags as tag_service from app.usage import events as usage_events logger = logging.getLogger(__name__) @@ -49,11 +52,13 @@ ) -def _prompt(asset: Asset, material: source.SourceMaterial) -> str: +def _prompt(asset: Asset, material: source.SourceMaterial, tag_names: list[str]) -> str: parts = [f"Filename: {asset.original_name or asset.name}"] if asset.description: # A description the user wrote is context, not something to restate. parts.append(f"The owner's own note about it: {asset.description}") + if tag_names: + parts.append("Tags already applied to this item: " + ", ".join(tag_names)) if material.kind == source.FROM_TRANSCRIPT: header = "Transcript of the recording" @@ -96,15 +101,16 @@ def run(session: Session, asset: Asset, progress: Progress) -> str: ) progress("Summarising", 30, detail) + tag_names = [t.name for t in tag_service.tags_for(session, asset.id)] completion = provider.complete( - _prompt(asset, material), + _prompt(asset, material, tag_names), system=SYSTEM_PROMPT, images=material.images, ) # Recorded before this job decides what to do with the answer, so usage does not - # depend on the outcome: a reply that gets trimmed, or that provenance then refuses - # to write, cost the same tokens as one that lands. + # depend on the outcome: a reply that gets trimmed still cost the same tokens as one + # that does not. # # Not a complete guarantee, and the gap is worth knowing: a reply the *provider's* # parser rejects — an empty completion, unusable JSON — raises before there is a @@ -123,11 +129,7 @@ def run(session: Session, asset: Asset, progress: Progress) -> str: # not still land a summary afterwards. progress("Saving", 90, "") - written = asset_service.apply_ai_metadata(session, asset, {"summary": text}) - if not written: - # Not an error. The user wrote their own summary, which outranks this one — but - # saying so beats a job that reports success and changed nothing. - return "Left alone — you wrote this summary yourself" + asset_service.apply_ai_metadata(session, asset, {"summary": text}) logger.info( "Summarised asset %s from %s (%d in / %d out tokens)", diff --git a/backend/app/services/assets.py b/backend/app/services/assets.py index a4b5f65..b9bd021 100644 --- a/backend/app/services/assets.py +++ b/backend/app/services/assets.py @@ -265,10 +265,11 @@ def delete_asset(session: Session, storage: LocalStorage, asset: Asset) -> None: def apply_metadata(session: Session, asset: Asset, changes: dict) -> Asset: """Apply a user's manual edits, recording that a human made them. - The provenance write is what FR 8.1.3 rests on: a later AI enrichment run reads it - to know which fields a person has touched, and must not overwrite those without - asking. Recording it here — at the only place manual edits happen — is what keeps - that guarantee from depending on every future caller remembering it. + Stamping "human" here still matters for `name`: a suggested title is only ever + applied through this path (`services/suggestions.py::accept`), and there is no + direct AI write path that could replace it afterwards. For `description` and + `summary` the stamp is now informational only — `apply_ai_metadata` no longer reads + it before overwriting either field. """ if not changes: return asset @@ -293,19 +294,15 @@ def apply_metadata(session: Session, asset: Asset, changes: dict) -> Asset: def apply_ai_metadata(session: Session, asset: Asset, changes: dict) -> list[str]: - """Write fields an enrichment job produced, without overwriting a person's work. + """Write fields an enrichment job produced. - The counterpart to `apply_metadata`, and the first thing to actually *read* - `field_provenance` — FR 8.1.3 requires that a later AI run never silently replaces - something someone typed, and until now the column was written and consulted by - nothing. + The counterpart to `apply_metadata`. Pressing a "Generate"/"Summarize"/"Describe" + control is a deliberate, explicit request for a fresh answer, so it always lands — + including over a value a person typed by hand. `field_provenance` is still stamped + "ai" afterwards, so the record of who wrote a field stays accurate even though + nothing here gates on it any more. - A field with no provenance entry has never been touched by a person, so it is free - to write: absence is the signal, and no third provenance value is needed for it. A - field marked "human" is skipped and named in the return value, so the caller can say - what it left alone rather than reporting a clean run that quietly did less. - - Returns the fields actually written. + Returns the fields written (always every key in `changes`, once any are given). """ if not changes: return [] @@ -315,16 +312,9 @@ def apply_ai_metadata(session: Session, asset: Asset, changes: dict) -> list[str except ValueError: provenance = {} - written: list[str] = [] for field_name, value in changes.items(): - if provenance.get(field_name) == "human": - continue setattr(asset, field_name, value) provenance[field_name] = "ai" - written.append(field_name) - - if not written: - return [] asset.field_provenance = json.dumps(provenance, sort_keys=True) asset.metadata_modified_date = utcnow() @@ -333,7 +323,7 @@ def apply_ai_metadata(session: Session, asset: Asset, changes: dict) -> list[str session.commit() session.refresh(asset) _reindex(session, asset) - return written + return list(changes.keys()) # ─── serialisation ─────────────────────────────────────────────────────────── diff --git a/backend/tests/test_assets_api.py b/backend/tests/test_assets_api.py index 53c549b..8894d64 100644 --- a/backend/tests/test_assets_api.py +++ b/backend/tests/test_assets_api.py @@ -206,7 +206,8 @@ def test_patch_updates_metadata_and_records_human_provenance(library, session): assert response.status_code == 200 assert response.json()["data"]["name"] == "Ocean at sunset" - # FR 8.1.3: a later AI run reads this to know what a person wrote. + # FR 8.1.3 bookkeeping: still recorded, though only `name` has anything left that + # reads it (a suggested title never overwrites one a person already chose). asset = session.get(Asset, created["id"]) import json diff --git a/backend/tests/test_describe.py b/backend/tests/test_describe.py index 6e37b11..c64e886 100644 --- a/backend/tests/test_describe.py +++ b/backend/tests/test_describe.py @@ -17,8 +17,9 @@ from app.providers import _upstream from app.providers.base import ProviderError from app.services import assets as asset_service +from app.services import tags as tag_service -from tests.test_summarize import _upload_image, add_transcript, configure_provider +from tests.test_summarize import TEST_USER, _upload_image, add_transcript, configure_provider DESCRIPTION = "James Giordano speaking to camera in a panelled studio, discussing DARPA." @@ -115,6 +116,18 @@ def test_an_existing_summary_is_context_not_material_to_repeat(library, session, assert "do not repeat it" in prompt +def test_the_assets_own_tags_are_given_as_context(library, session, upstream): + created = _upload_image(library) + asset = session.get(Asset, created["id"]) + tag = tag_service.get_or_create(session, TEST_USER, "DARPA") + tag_service.attach(session, asset.id, tag.id) + configure_provider(session) + + describe.run(session, asset, lambda *a, **k: None) + + assert "DARPA" in sent_prompt(upstream) + + # ─── the ordinary paths ────────────────────────────────────────────────────── @@ -139,17 +152,19 @@ def test_the_description_lands_on_the_asset(library, session, upstream): assert json.loads(asset.field_provenance)["description"] == "ai" -def test_it_refuses_to_overwrite_a_description_someone_wrote(library, session, upstream): +def test_a_description_someone_wrote_is_replaced_when_asked(library, session, upstream): + """Pressing Describe is an explicit request for a fresh answer, so it always lands + — even over a description a person typed themselves.""" created = _upload_image(library) asset = session.get(Asset, created["id"]) asset_service.apply_metadata(session, asset, {"description": "Mine, thanks."}) configure_provider(session) - detail = describe.run(session, asset, lambda *a, **k: None) + describe.run(session, asset, lambda *a, **k: None) session.refresh(asset) - assert asset.description == "Mine, thanks." - assert "you wrote this" in detail.lower() + assert asset.description == DESCRIPTION + assert json.loads(asset.field_provenance)["description"] == "ai" def test_an_over_long_reply_is_trimmed(library, session, upstream): diff --git a/backend/tests/test_summarize.py b/backend/tests/test_summarize.py index 6f9a806..d37a002 100644 --- a/backend/tests/test_summarize.py +++ b/backend/tests/test_summarize.py @@ -2,7 +2,8 @@ Two behaviours matter more than the rest and have most of the cases below: what the job chooses to read (a transcribed video is summarised from its transcript, never from one -frame of it), and what it refuses to overwrite (a summary somebody typed). +frame of it), and that pressing the button always lands a fresh answer — even over a +summary somebody typed by hand. """ import json @@ -23,6 +24,7 @@ from app.providers import _upstream from app.providers.base import ProviderError from app.services import assets as asset_service +from app.services import tags as tag_service FIXTURES = Path(__file__).parent / "fixtures" TEST_USER = "user-under-test" @@ -225,6 +227,18 @@ def test_the_owners_own_note_is_given_as_context(library, session, upstream): assert "Shot on the roof in Lisbon." in sent_prompt(upstream) +def test_the_assets_own_tags_are_given_as_context(library, session, upstream): + created = _upload_image(library) + asset = session.get(Asset, created["id"]) + tag = tag_service.get_or_create(session, TEST_USER, "DARPA") + tag_service.attach(session, asset.id, tag.id) + configure_provider(session) + + summarize.run(session, asset, lambda *a, **k: None) + + assert "DARPA" in sent_prompt(upstream) + + # ─── what it writes ────────────────────────────────────────────────────────── @@ -240,19 +254,19 @@ def test_the_summary_lands_on_the_asset(library, session, upstream): assert json.loads(asset.field_provenance)["summary"] == "ai" -def test_it_refuses_to_overwrite_a_summary_someone_wrote(library, session, upstream): - """FR 8.1.3. `field_provenance` was written from M1 and read by nothing until now.""" +def test_a_summary_someone_wrote_is_replaced_when_asked(library, session, upstream): + """Pressing Summarize is an explicit request for a fresh answer, so it always lands + — even over a summary a person typed themselves.""" created = _upload_image(library) asset = session.get(Asset, created["id"]) asset_service.apply_metadata(session, asset, {"summary": "Mine, thanks."}) configure_provider(session) - detail = summarize.run(session, asset, lambda *a, **k: None) + summarize.run(session, asset, lambda *a, **k: None) session.refresh(asset) - assert asset.summary == "Mine, thanks." - # Reported rather than silently doing nothing and claiming success. - assert "you wrote this" in detail.lower() + assert asset.summary == SUMMARY + assert json.loads(asset.field_provenance)["summary"] == "ai" def test_a_previous_ai_summary_is_replaced(library, session, upstream): @@ -302,8 +316,9 @@ def test_ai_metadata_writes_an_untouched_field(library, session): assert json.loads(asset.field_provenance) == {"summary": "ai"} -def test_ai_metadata_skips_only_the_human_field(library, session): - """A mixed write should land the parts it is allowed to, not abort wholesale.""" +def test_ai_metadata_overwrites_a_field_marked_human(library, session): + """A field someone edited by hand is still fair game for the next AI write — + pressing Generate again is what asked for this.""" created = _upload_image(library) asset = session.get(Asset, created["id"]) asset_service.apply_metadata(session, asset, {"description": "Mine."}) @@ -312,21 +327,11 @@ def test_ai_metadata_skips_only_the_human_field(library, session): session, asset, {"description": "theirs", "summary": "auto"} ) - assert written == ["summary"] + assert written == ["description", "summary"] session.refresh(asset) - assert asset.description == "Mine." + assert asset.description == "theirs" assert asset.summary == "auto" - - -def test_ai_metadata_with_nothing_allowed_writes_nothing(library, session): - created = _upload_image(library) - asset = session.get(Asset, created["id"]) - asset_service.apply_metadata(session, asset, {"summary": "Mine."}) - before = asset.metadata_modified_date - - assert asset_service.apply_ai_metadata(session, asset, {"summary": "auto"}) == [] - session.refresh(asset) - assert asset.metadata_modified_date == before + assert json.loads(asset.field_provenance)["description"] == "ai" # ─── the endpoint ──────────────────────────────────────────────────────────── diff --git a/backend/tests/test_usage.py b/backend/tests/test_usage.py index cfb59b5..1f7818e 100644 --- a/backend/tests/test_usage.py +++ b/backend/tests/test_usage.py @@ -142,22 +142,6 @@ def test_a_provider_reporting_no_tokens_records_nothing(library, session, upstre assert session.exec(select(UsageEvent)).all() == [] -def test_usage_does_not_depend_on_what_happens_to_the_answer(library, session, upstream): - """Provenance refuses the write, but the tokens were still spent. A total that only - counted runs whose output landed would understate what the library cost.""" - from app.services import assets as asset_service - - created = _upload_image(library) - asset = session.get(Asset, created["id"]) - asset_service.apply_metadata(session, asset, {"summary": "Mine, thanks."}) - configure_provider(session) - - detail = summarize.run(session, asset, lambda *a, **k: None) - - assert "you wrote this" in detail.lower() - assert len(session.exec(select(UsageEvent)).all()) == 1 - - def test_a_reply_the_provider_parser_rejects_is_not_recorded(library, session, upstream): """The one gap, asserted rather than left to be discovered. An empty completion raises inside the client, before this job holds a Completion to record — so those diff --git a/docs/m6-ai-enrichment.md b/docs/m6-ai-enrichment.md index 2bfc0db..6265d27 100644 --- a/docs/m6-ai-enrichment.md +++ b/docs/m6-ai-enrichment.md @@ -210,18 +210,37 @@ Two decisions that go with it: user sees in their library without asking is a different act from filling a blank. It goes through the same accept/reject path as `autotag` (FR 9.1.4). -### What protects a human edit +### What `field_provenance` records -`field_provenance` is written in exactly one place — `services/assets.py::apply_metadata`, -which stamps `"human"` — and read nowhere. A freshly ingested asset therefore has `{}`. - -That absence is already the signal step 7 needs, so **no new provenance value is -required**: a field with no entry has never been touched by a person and an AI run may -write it; a field marked `"human"` may not be overwritten without asking. An AI write -stamps `"ai"`, which a later AI run is free to replace. +**Amended post-M6, once real use showed the original rule did not hold up. Read this in +place of the "What protects a human edit" section it replaces; the rest of this document +still reflects the design as landed.** -`name` is the exception, and not because of provenance: it is never blank, so the -suggestion rule above applies to it whether or not a person has edited it. +`field_provenance` is written in exactly one place — `services/assets.py::apply_metadata`, +which stamps `"human"` — and, as of M6, was read by `apply_ai_metadata` to *refuse* to +overwrite a `"human"` field. That turned out to be the wrong default: pressing +Summarize/Describe/"Generate all" is already a deliberate, explicit request for a fresh +answer, and a silent no-op in response to it read as broken rather than protective — +doubly so because the field it refused to touch was often one the person had never +actually typed into (see below). + +So `apply_ai_metadata` no longer checks provenance before writing. Every enrichment write +lands, unconditionally, including over a value a person wrote by hand. The column is +still stamped — `"human"` from a manual edit, `"ai"` from a generated one — purely as a +record of who last wrote a field, for the "surfacing provenance in the UI" idea this +document originally deferred. Nothing currently gates on it. + +The other half of why the old rule bit harder than intended: the asset panel's single +"Save changes" button sends `name`, `description` and `summary` together in one request +whenever any one of them changed, so editing just the name also re-sent the other two +fields' current values — which the API cannot distinguish from a deliberate edit, and so +stamped `"human"` on fields the person had never touched, including ones still blank. +That collateral marking is fixed alongside this change; provenance now reflects only the +field actually edited. + +`name` remains the one field the suggestion rule (accept/reject, never a direct write) +applies to regardless of provenance, because it is never blank rather than because of +anything provenance records. --- @@ -355,14 +374,13 @@ backend/app/ Rejections are kept rather than deleted, so a re-run does not propose a tag the user already declined. Accepting a title goes through `apply_metadata` — the *human* path — because the user read it and chose it, which also stops a later run replacing it. -7. `field_provenance` enforcement — **mostly done**, arriving with step 4 because - `summarize` was the first AI write path and shipping it without this would have meant - shipping the bug the rule exists to prevent. `services/assets.py::apply_ai_metadata` - reads the column, writes fields with no entry, skips ones marked `"human"`, and - returns what it actually wrote. No new provenance value was needed; an absent entry - already means "no person has touched this". What remains: applying it to `describe`'s - write path when that lands, and surfacing provenance in the UI so a user can see which - fields the AI wrote. +7. `field_provenance` enforcement — **landed with step 4, then reverted post-M6.** + `apply_ai_metadata` originally read the column and skipped a field marked `"human"`. + In practice that made "Generate"/"Summarize"/"Describe" silently do nothing on a + field the person had often never actually typed into — see "What `field_provenance` + records" above for why, and for what it does now instead: every explicit press + overwrites unconditionally, and the column is kept only as a record of who wrote a + field, not as a gate. 8. ~~Bulk enrichment over a selection — including the `SelectionBar` embed deferred from M5.~~ **Done.** One `bulk_enrich` job over the whole selection rather than one job per asset: cancellation is per row, so N jobs would mean N Cancel clicks and N diff --git a/docs/plan-of-attack.md b/docs/plan-of-attack.md index a1aed58..c9681a4 100644 --- a/docs/plan-of-attack.md +++ b/docs/plan-of-attack.md @@ -137,8 +137,11 @@ class Asset(SQLModel, table=True): Two deliberate deviations from the SRS schema: -- **`field_provenance`** implements FR 8.1.3 ("manual edits are never overwritten by a - later AI run without confirmation"). Without it that requirement has no mechanism. +- **`field_provenance`** was built for FR 8.1.3 ("manual edits are never overwritten by a + later AI run without confirmation"), but no longer enforces it — see `docs/m6-ai- + enrichment.md`'s "What `field_provenance` records" for why a silent skip turned out to + be the wrong default. The column still records `"human"` vs `"ai"` per field; nothing + currently gates on it. - **Transcripts are rows, not a nested JSON blob.** `TranscriptSegment(id, asset_id, user_id, idx, text, start_time, end_time, words JSON)`. FR 10.1.4 requires returning *a matching snippet with its timestamp*; that means each segment must be individually diff --git a/frontend/src/components/AssetDetail.tsx b/frontend/src/components/AssetDetail.tsx index ad15411..012d835 100644 --- a/frontend/src/components/AssetDetail.tsx +++ b/frontend/src/components/AssetDetail.tsx @@ -12,7 +12,7 @@ import { Wand2, X, } from 'lucide-react' -import type { Asset } from '@/api/assets' +import type { Asset, AssetUpdate } from '@/api/assets' import { tagsApi } from '@/api/tags' import { enrichmentApi } from '@/api/enrichment' import { formatCost, usageApi, type UsageTotals } from '@/api/usage' @@ -199,11 +199,17 @@ export default function AssetDetail({ if (!dirty || !name.trim()) return setSaving(true) try { - await update(asset.id, { - name: name.trim(), - description: description || null, - summary: summary || null, - }) + // Only the fields that actually changed: the API marks every key it receives as + // human-written provenance (FR 8.1.3 bookkeeping), so sending name/description/ + // summary as a fixed trio would stamp the two you didn't touch right alongside + // the one you did — including while they're still empty. + const changes: AssetUpdate = {} + if (name.trim() !== asset.name) changes.name = name.trim() + if (description !== (asset.description ?? '')) + changes.description = description || null + if (summary !== (asset.summary ?? '')) changes.summary = summary || null + + await update(asset.id, changes) } catch { // The store restores the server's version and surfaces the message; the panel // stays open so the edit is not lost. diff --git a/frontend/src/components/EnrichmentButton.tsx b/frontend/src/components/EnrichmentButton.tsx index 0a617a4..6534022 100644 --- a/frontend/src/components/EnrichmentButton.tsx +++ b/frontend/src/components/EnrichmentButton.tsx @@ -94,8 +94,7 @@ export default function EnrichmentButton({ .

)} - {/* A finished job's own message, which is where "you wrote this yourself" and any - provider failure arrive. */} + {/* A finished job's own message, which is where a provider failure arrives. */} {!running && job?.status === 'error' && job.error_message && (

{job.error_message}

)}