Skip to content

fix: delete and restore name the ids they skip, like archive - #387

Merged
yw0nam merged 4 commits into
mainfrom
fix/340-missing-ids
Oct 21, 2026
Merged

yw0nam merged 4 commits into
mainfrom
fix/340-missing-ids

Conversation

@yw0nam

@yw0nam yw0nam commented Oct 21, 2026

Copy link
Copy Markdown
Owner

Summary

/admin/notes/delete and /admin/restore refuse an id they cannot find the way /admin/archive does: a 400 that counts the ids and lists them as missing. The check runs before both the preview and confirm, so nothing changes when one id is wrong.

routes.py
  _refuse_missing(ids, rows, noun) -> 400 {"error": "<n> of <m> ids are not <noun>: ...", "missing": [...]} | None
  archive_route  -> _refuse_missing(..., "active notes")   (response unchanged)
  delete_route   -> _refuse_missing(..., "agent notes")    (was: "ids must refer only to agent_note rows", no list)
  restore_route  -> rows_by_ids(ids) before preview and confirm,
                    _refuse_missing(..., "stored rows")    (was: unknown id silently skipped)

restore still looks rows up without notes_only, so archived document chunks stay restorable. The MCP tools delete_notes and restore_notes relay the REST error string unchanged, so agents see the ids without a tool change. The README rows for the two routes state the refusal.

Not changed here: link_entities reports its own refusal under the key ids, not missing.

Related issues

Closes #340

Runtime evidence (required for ingest / retrieval / serving behavior)

Real REST app against the throwaway Postgres container (a temporary integration test, not committed), with one seeded agent note and one unknown id:

/admin/notes/delete {}                -> 400 {"error":"1 of 2 ids are not agent notes: note:default:doesnotexist","missing":["note:default:doesnotexist"]}
/admin/notes/delete {"confirm": true} -> 400 {"error":"1 of 2 ids are not agent notes: note:default:doesnotexist","missing":["note:default:doesnotexist"]}
/admin/archive {"confirm": true}      -> 400 {"error":"1 of 2 ids are not active notes: note:default:doesnotexist","missing":["note:default:doesnotexist"]}
/admin/restore {}                     -> 400 {"error":"1 of 2 ids are not stored rows: note:default:doesnotexist","missing":["note:default:doesnotexist"]}
/admin/restore {"confirm": true}      -> 400 {"error":"1 of 2 ids are not stored rows: note:default:doesnotexist","missing":["note:default:doesnotexist"]}
/admin/notes/delete known id only     -> 200 {"deleted":1}

RED before the fix (-k cannot_find): KeyError: 'missing' (delete ×2), assert 200 == 400 (restore ×2), 4 failed.

Verification

  • uv run pytest passes: unit tests/serve 1051 passed, 1 skipped; integration test_archive_flow.py, test_facts_integration.py, test_consolidate_verdicts_integration.py 19 passed
  • uv run ruff format --check . and uv run ruff check . pass
  • docker compose build mcp passes (Dockerfile / dependency changes) — N/A, no dependency change
  • Runtime evidence attached above, or N/A justified

Checklist (core principles)

  • Selective storage — notes stay distilled; verbatim text lives only in doc_rows and the turns lane
  • Source-specific logic stays out of the source-agnostic core (ingest pipeline, retrieval, serving)
  • No hardcoded endpoints/models — configuration lives in .env
  • New/changed behavior ships a test in this PR (or skip-tests label justified)

@yw0nam
yw0nam merged commit ec6f95d into main Oct 21, 2026
4 checks passed
@yw0nam
yw0nam deleted the fix/340-missing-ids branch October 21, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: delete_notes and restore_notes do not name the ids they skip, unlike archive_notes

1 participant