Deduplicate sender detection — 70+ duplicate noreply@github.com rows #7
Loading…
Reference in a new issue
No description provided.
Delete branch "claude/wi-019e535b-deduplicate-sender-detection-70-duplicat"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Draft PR for Anima work item Deduplicate sender detection — 70+ duplicate noreply@github.com rows (
019e535b-0b68-7e72-8b44-f9a0bbf49200).This PR is a scaffold opened by
anima-triage. The implementation worker picks it up from here..anima/wip/019e535b.mdtriagedClose or mark ready manually once the implementation lands.
6563f49c6407680a160a07680a160aa6e039b630a6e039b630a79c8d30aea79c8d30aea17ccc7441a17ccc74414e7dd1ac404e7dd1ac40c52ec15ba7c52ec15ba77eac090ae07eac090ae042a9d26ddf42a9d26ddf084c060883084c0608833ee5a7b1023ee5a7b102188798416618879841669011e453069011e45306fdbf90b403fdbf90b40390541bd46990541bd46942dbc5da0dParity first: most of WI-5 is already satisfied on main and did not need rebuilding. - "check for existing person by email before creating one" — add_person already looks up person_emails and returns the existing person. - "upsert pattern" — achieved equivalently by a transactional check-then-insert, so a failed person_emails insert can no longer strand a people row. - unique constraint on the email column — present since the initial commit (m20260427_000001_init), so every deployed database already has it. The earlier attempt on PR #7 re-added that unique constraint as a new migration, which is redundant against main. What was genuinely missing is the "source" half of the work item's enrichment clause. `AgentAction::PeopleUpsertPending` carries a `source`, the executor logs it, and then `add_person` threw it away and hardcoded "manual" for every caller. Every automatically detected sender was therefore recorded as though a human had typed it, and the distinction the column exists to hold was lost. - add_person now takes an EmailSource. The executor's notification path records Auto; the person.add tool records Manual. - Enrichment on an address we already hold is monotonic: display_name is still only filled when absent, and source is upgraded auto -> manual when a human registers an address we had only inferred. It is never downgraded — a later automatic sighting must not erase that a human vouched for it. - source_label() is the single stored spelling, so the insert path and the enrichment comparison cannot drift. On the remaining duplicate cleanup: WI-5's own triage left "should the migration actively delete or merge the existing duplicates, and how should we reassign their foreign keys?" as an open question, and it was never answered. PR #7 answered it unilaterally with DELETE FROM people WHERE id NOT IN (SELECT DISTINCT person_id FROM person_emails) against production. That is a data-loss decision, not an implementation detail, so it is deliberately not taken here. Instead `person.duplicates` reports the population so the decision can be made on real numbers: addresses claimed by more than one people row (possible because people.primary_email carries no unique constraint, even though person_emails.email does), and people rows with no person_emails entry — the strays from the historical insert order, which the current transactional add_person can no longer create. The tool is read-only and takes no arguments; it performs no merge and no delete. Only person_emails references people, so a future merge has exactly one table to reassign. Tests (5 new, all passing): the stored source labels are pinned as wire values because the enrichment path compares against them; the upgrade rule is asserted in all four directions including the two no-ops and the forbidden downgrade; the report round-trips through the MCP surface; the census is advertised argument-free; person.add registers as Manual. Verified: cargo test -p sia-store -p sia-mcp --lib 32/32; cargo clippy -p sia-store -p sia-mcp -p sia-agent --all-targets clean; cargo fmt --all --check clean; cargo check --workspace clean. No migration, no schema change, no production data touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>2b902620into the recovery branch`get_project_ci_status` reported `resolvable: false` for Sia — no `default_ci_steps` and no `.solstice/job.sh` — so every native dispatch failed at config resolution before running anything. That failure is not neutral. anima-server's solstice.rs routes it to `handle_config_failure`, which parks the work item with `ParkedReason::CiConfigUnresolvable`. The ABSENCE of this file reads as "this work item's CI is broken" and parks real work. akh-medu learned this the expensive way (8/8 native dispatches failed, the WI 1/2/3 series parked) and its job.sh states the rule: a missing file must not read as "the code is red". Sia had CI only on the Forgejo-Actions lane, so the native lane — the one Anima's exact-head gate reads — had never produced a verdict at all. One definition of the checks, two lanes --------------------------------------- scripts/ci-checks.sh is now the only place the cargo commands are written. Both .solstice/job.sh and .forgejo/workflows/ci.yml call it; neither re-lists them. akh-medu's rule is that a native verdict must never be red for work the Actions lane would call green, because the disagreement parks work items on phantom failures. Two inline copies drift the first time someone edits one; sharing the script makes that structurally impossible rather than a reviewer's responsibility. job.sh only provisions. It deliberately differs from the Anima/akh-medu scripts in two places: - It does not assume passwordless sudo. Those scripts call `sudo apt-get` unconditionally because their image grants it; a runner without sudo would fail with a bare "sudo: command not found" that classifies as a code failure and parks the item. Privilege is resolved once, only missing packages are installed, and the no-privilege case exits with a message naming the packages and the reason. - It does not `rm -rf ../anima`. The workflow did, and on a persistent or shared runner that deletes a checkout the job does not own. An existing sibling is reused; a non-git path there is refused rather than removed. The resolved revision is always logged so an upstream anima-proto change is attributable. ANIMA_SIBLING_REV pins a run without freezing CI to a revision the shipped image does not use. protoc keeps ci.yml's pinned 25.3 release, since Ubuntu's 3.12 rejects anima-proto's proto3 `optional` fields, and installs into the checkout so it needs no privileges. The build is debug: release codegen is what OOM-killed rustc on the Solstice VMs for akh-medu, and an OOM reads as red on healthy code. Sia's release build is still covered by the `docker` job on push to main. Validation ---------- scripts/test-ci-job.sh replaces every external command with a logging stub and asserts on the control flow — the branches that only execute on a runner in an unusual state, i.e. never until the day they matter. 10 scenarios, 36 assertions, all passing; it needs no toolchain and runs in seconds, so it is wired into ci-checks.sh and guards the CI definition on both lanes. It earned its keep immediately, catching three defects before any runner saw the script: `"${profile_args[@]}"` on an empty array is an unbound-variable error under `set -u` on bash < 4.4, so the script would have worked on the runner but not locally, defeating the point of sharing it; the openssl probe hardcoded /usr/include/openssl/ssl.h and reported a false "missing" on any other layout, triggering a pointless privileged install; and the harness's own no-sudo case was inheriting the host's cached sudo credentials, leaving the no-privilege branch untested exactly where it was most likely to be wrong. No application change: no Cargo.toml edit, no dependency version change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>e5ead22All four were real. Reproduced each before fixing. 1. The install died as root. `$SUDO DEBIAN_FRONTEND=noninteractive apt-get` looks like an assignment prefix but is not one: bash decides that from the text BEFORE expansion, and the first word is `$SUDO`. Once that expands to nothing — i.e. running as root, the case that should always work — the assignment becomes the command name: $ SUDO=""; $SUDO DEMO_ONLY=1 true bash: DEMO_ONLY=1: command not found (rc=127) Routed through `env`, which is a real command in both shapes. The harness now exercises root explicitly via an `id` stub rather than only covering sudo/non-root, so the escalation-free path has real coverage. 2. A git worktree's `.git` is a FILE holding a gitdir pointer, so `[ -d .git ]` rejected a perfectly valid shared worktree — and the next branch then refused to run at all. This checkout is itself a worktree, so the script would have rejected the very tree it was written in. Now asks git (`rev-parse --is-inside-work-tree`) and confirms the path is that worktree's ROOT by comparing realpaths, so a subdirectory of an unrelated repo above it is still refused — non-destructively. 3. ANIMA_SIBLING_REV claimed a pin it never enforced: it was only applied on the clone path, so with an existing checkout the requested revision was silently ignored and the verdict was attributed to a revision that was never built. Now validated against the existing checkout and REFUSED on mismatch, without touching a checkout this job does not own. An unknown rev is likewise refused rather than fetched into it. ANIMA_SIBLING_DIR is gone. It was misleading: cargo reads the override path from the workspace [patch] table, so pointing the variable elsewhere provisioned a directory cargo would never look at while the build still failed on the real path. The sibling location is now derived from Cargo.toml, which is the only thing that actually decides it, and a missing [patch] path fails loudly instead of guessing. 4. `clippy --workspace --all-targets -- -D warnings` was stricter than the ci.yml it replaced (libs/bins only). Pulling tests, benches and examples into a -D warnings gate for the first time risks an unrelated baseline failure landing as a red verdict on unrelated work — the phantom-failure park this work item exists to stop. Restored to the previously-verified scope. Widening is a separate change that needs a clean baseline run and then moves both lanes together. Also fixes unbounded recursion introduced when the harness was wired into ci-checks.sh: the harness runs job.sh, which execs ci-checks.sh, which ran the harness. Guarded with CI_SKIP_JOB_HARNESS, which the harness sets for its children, and the guard itself is now covered. Harness: 18 scenarios, 62 assertions, all passing. New fixtures are real, not mocked around — an actual shared worktree created with `git worktree add`, an actual non-root checkout, actual commit SHAs for the pin cases, and a real Cargo.toml with a [patch] table so the derivation runs the same code path the runner takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>WIP: Deduplicate sender detection — 70+ duplicate noreply@github.com rowsto Deduplicate sender detection — 70+ duplicate noreply@github.com rowsView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.