Feat/merge review staleness - #259
AlexAxthelm wants to merge 3 commits into
Conversation
When an overlapping merge candidate is approved, a member of another pending
candidate gets merged away (repointed). Previously that candidate's Approve was
guaranteed to fail with a message interpolating repr() on ORM objects
("Repointed: [<...object at 0x...>]") — no id, no pointer to where the resource
went — and the only exit was Deny, recording a review judgment no one made.
Surface the move instead: resolve repointed member ids to their terminal
resource and show the curator where each one went. Completes AC 3 of STIT-418
Backend:
- ResourceModel._parent_tree_cte now emits (origin_id, id) so a batch keeps
each input mapped to its own root instead of returning an unkeyed set of
roots; add root_id_by_resource_id(), a batched dict-returning resolver.
get_root/_root_select are unchanged (documented single-id).
- Add RepointedResourceView and a repointed_resources list on MergeCandidateView
(plain field, not computed_field, so the detail view's model_dump rebuild
keeps it). Populated only for PENDING candidates via _resolve_candidates,
which does a single root lookup over the union of member ids (the queue is
unpaginated, so per-candidate resolution would be an N+1).
- Replace the repr() merge-guard message with a shared repointed_merge_error()
that names each id and its terminal target ("resource 102 is now resource
301"), wired into both the merge-candidate and write-path guards.
Frontend:
- New utils/mergeCandidateStaleness.js: the single reader of repointed_resources,
degrading to "not stale" when the field is absent.
- Merge review pane shows a warning banner naming each move (both ids linked),
hides the decision controls when stale (Approve would fail; Deny would record
a false judgment), and annotates a moved source resource as "(now 301)".
Tests:
- Backend: origin-column batch mapping, the N+1 batch guard, the PENDING-only
rule, the cross-candidate stale case end-to-end (queue + detail report the
move; re-approve 400s naming both ids with no "object at 0x", stays PENDING),
and fill the previously-empty apply_resource_merge guard stub.
- Frontend: stale banner + hidden controls, the facts annotation, a compat guard
(payload without the field renders as before), and a pure-function suite for
the staleness util.
The retire/requeue/SUPERSEDED-status flow is intentionally deferred (see PLAN.md)
until the stale-candidate dead end actually blocks a reviewer, keeping this PR
purely additive and rollback-safe.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0abc86e to
3a61211
Compare
CD summary
|
| service | url | fqdn |
|---|---|---|
| api | open | pr-0259-api.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| entity-linkage | open | pr-0259-el.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| frontend | https://witty-mushroom-017a3dc1e-259.westus2.1.azurestaticapps.net | |
| stitch-llm | open | pr-0259-llm.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
Database (1)
| db_name | postgres_host | postgres_port | postgres_db |
|---|---|---|---|
| pr_0259 | stitch-dev.postgres.database.azure.com |
5432 |
pr_0259 |
Jobs (2)
| job | image | postgres_db | api_url | auth_mode |
|---|---|---|---|---|
| db-migrations | ghcr.io/rmi/stitch-api:pr-0259@sha256:6d9b4dc7ec697ec57a548f3ddd3785d3c8498e3b2b830fb8015757143d3e61a0 |
pr_0259 |
||
| seed | ghcr.io/rmi/stitch-seed:pr-0259@sha256:8132cd8b82c8eb988cbc9abec651f74940391fa52a7eb37efce3450b80df67c9 |
https://pr-0259-api.purplegrass-c07d0a94.westus2.azurecontainerapps.io/api/v1 |
stitch-client-bearer-token |
Images (4)
| build_time | commit_time | git_sha | image | image_digest |
|---|---|---|---|---|
| 2026-08-28T16:21:11Z | 2026-08-28T16:20:54Z | f7f223f | ghcr.io/rmi/stitch-api:pr-0259 |
ghcr.io/rmi/stitch-api:pr-0259@sha256:6d9b4dc7ec697ec57a548f3ddd3785d3c8498e3b2b830fb8015757143d3e61a0 |
| 2026-08-28T16:21:11Z | 2026-08-28T16:20:54Z | f7f223f | ghcr.io/rmi/stitch-entity-linkage:pr-0259 |
ghcr.io/rmi/stitch-entity-linkage:pr-0259@sha256:84e4db2403c9d16e701df08e14ce3bc7a1bc24b06d2da8aacba6c08f7ad6b18a |
| 2026-08-28T16:21:11Z | 2026-08-28T16:20:54Z | f7f223f | ghcr.io/rmi/stitch-seed:pr-0259 |
ghcr.io/rmi/stitch-seed:pr-0259@sha256:8132cd8b82c8eb988cbc9abec651f74940391fa52a7eb37efce3450b80df67c9 |
| 2026-08-28T16:21:11Z | 2026-08-28T16:20:54Z | f7f223f | ghcr.io/rmi/stitch-stitch-llm:pr-0259 |
ghcr.io/rmi/stitch-stitch-llm:pr-0259@sha256:f79d040427b9b0af22516f2c48b95aa61ca64b00986c24f513479b1d1e3fe9f7 |
…(STIT-418) - get_resolved loads the resource row once and, for the common non-repointed case, builds the entity from it directly instead of going through resolve_root_id + get() (which re-fetched the same id). A repointed id still resolves via resolve_root_id (which maps a broken chain to 404) and loads the terminal resource. - create_merge_candidate returns the view with an empty staleness resolution instead of running the root-lookup CTE: its members are validated as unrepointed immediately above, so the lookup could only ever return empty. (approve/deny short-circuit on their terminal status.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CD summary
|
| service | url | fqdn |
|---|---|---|
| api | open | pr-0259-api.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| entity-linkage | open | pr-0259-el.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| frontend | https://witty-mushroom-017a3dc1e-259.westus2.1.azurestaticapps.net | |
| stitch-llm | open | pr-0259-llm.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
Database (1)
| db_name | postgres_host | postgres_port | postgres_db |
|---|---|---|---|
| pr_0259 | stitch-dev.postgres.database.azure.com |
5432 |
pr_0259 |
Jobs (1)
| job | image | postgres_db |
|---|---|---|
| db-migrations | ghcr.io/rmi/stitch-api:pr-0259@sha256:cfa9e21f3f572cbb4c2726f226abdc53ed9a0d13e4879762c29efde8f59c96a7 |
pr_0259 |
Images (4)
| build_time | commit_time | git_sha | image | image_digest |
|---|---|---|---|---|
| 2026-08-28T17:01:20Z | 2026-08-28T17:01:00Z | 388fa91 | ghcr.io/rmi/stitch-api:pr-0259 |
ghcr.io/rmi/stitch-api:pr-0259@sha256:cfa9e21f3f572cbb4c2726f226abdc53ed9a0d13e4879762c29efde8f59c96a7 |
| 2026-08-28T17:01:19Z | 2026-08-28T17:01:00Z | 388fa91 | ghcr.io/rmi/stitch-entity-linkage:pr-0259 |
ghcr.io/rmi/stitch-entity-linkage:pr-0259@sha256:af706928bf511c0f0437fec2afcd36cbe3f1d0f3a0b9a0ab768f5c51781874dd |
| 2026-08-28T17:01:19Z | 2026-08-28T17:01:00Z | 388fa91 | ghcr.io/rmi/stitch-seed:pr-0259 |
ghcr.io/rmi/stitch-seed:pr-0259@sha256:85bb8793994aee34409b738bcab02b65f107239866e332cb4044c6a71268b371 |
| 2026-08-28T17:01:23Z | 2026-08-28T17:01:00Z | 388fa91 | ghcr.io/rmi/stitch-stitch-llm:pr-0259 |
ghcr.io/rmi/stitch-stitch-llm:pr-0259@sha256:ee03a011a1403b4a1a97c1dfc33eca2169a5b44ba810d02d0e08b04e96308546 |
There was a problem hiding this comment.
🟡 Changes recommended
get_resolved still re-queries the same resource on the repointed path via resolve_root_id, which is an avoidable extra round-trip and undermines the intended perf improvement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the merge-review workflow to detect and surface “stale” merge candidates caused by overlapping candidates where one approval repoints a resource that another pending candidate still references. It adds an explicit repointed_resources signal to merge-candidate API views, blocks re-approval with clearer id-based error messages, and updates the Merge Review UI to warn reviewers and hide actions when a candidate has become invalid.
Changes:
- Backend: add
repointed_resourcesto merge-candidate views (computed in a batched root lookup for PENDING candidates) and improve merge guards to return id-based “resource X is now resource Y” errors. - Frontend: add a staleness utility and update the Merge Review pane to show a warning banner, annotate moved members, and hide Approve/Deny controls for stale candidates.
- Tests: add/extend unit + integration tests covering batch resolution, PENDING-only behavior, overlapping-candidate staleness, and merge guard messaging.
File summaries
| File | Description |
|---|---|
| deployments/stitch-frontend/src/utils/mergeCandidateStaleness.js | Adds a single utility to interpret repointed_resources into {isStale, moves}. |
| deployments/stitch-frontend/src/utils/mergeCandidateStaleness.test.js | Adds a pure-function test suite for the staleness utility. |
| deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.jsx | Renders stale-candidate banner, annotates moved members, and hides decision controls when stale. |
| deployments/stitch-frontend/src/pages/MergeCandidateReviewPage.test.jsx | Adds UI tests for stale banner, hidden controls, and facts annotations. |
| deployments/api/src/stitch/api/entities.py | Introduces RepointedResourceView and adds repointed_resources to MergeCandidateView. |
| deployments/api/src/stitch/api/db/og_field_resource_actions.py | Improves repointed-merge error messaging and optimizes get_resolved read path. |
| deployments/api/src/stitch/api/db/model/resource.py | Adds batched root_id_by_resource_id and augments the recursive CTE to preserve origin-to-root mapping. |
| deployments/api/src/stitch/api/db/merge_candidate_actions.py | Computes and propagates repointed_resources for candidates (batched for list endpoint; resolved for detail/actions). |
| deployments/api/tests/test_merge_candidate_actions.py | Updates unit tests and adds tests for batched resolution and terminal-status skip logic. |
| deployments/api/tests/routers/test_merge_candidates_integration.py | Adds end-to-end integration test for overlapping-approval staleness and id-based 400 messaging. |
| deployments/api/tests/db/test_resource_actions.py | Adds integration tests for root_id_by_resource_id mapping correctness and empty-input behavior. |
| deployments/api/tests/db/actions/test_merge.py | Adds guard test ensuring apply-merge rejects repointed inputs with id-based message (no repr() leakage). |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if model.repointed_id is not None: | ||
| root_id = await resolve_root_id(session, id) | ||
| return await get(session, root_id, licensed_sources=licensed_sources) |
CD summary
|
| service | url | fqdn |
|---|---|---|
| api | open | pr-0259-api.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| entity-linkage | open | pr-0259-el.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
| frontend | https://witty-mushroom-017a3dc1e-259.westus2.1.azurestaticapps.net | |
| stitch-llm | open | pr-0259-llm.purplegrass-c07d0a94.westus2.azurecontainerapps.io |
Database (1)
| db_name | postgres_host | postgres_port | postgres_db |
|---|---|---|---|
| pr_0259 | stitch-dev.postgres.database.azure.com |
5432 |
pr_0259 |
Jobs (1)
| job | image | postgres_db |
|---|---|---|
| db-migrations | ghcr.io/rmi/stitch-api:pr-0259@sha256:4e84bbebc59552192978814d3b27c0a0d624161bd705680d12160d1dc256d94c |
pr_0259 |
Images (4)
| build_time | commit_time | git_sha | image | image_digest |
|---|---|---|---|---|
| 2026-09-04T13:17:33Z | 2026-09-04T13:17:13Z | bdde2c3 | ghcr.io/rmi/stitch-api:pr-0259 |
ghcr.io/rmi/stitch-api:pr-0259@sha256:4e84bbebc59552192978814d3b27c0a0d624161bd705680d12160d1dc256d94c |
| 2026-09-04T13:17:31Z | 2026-09-04T13:17:13Z | bdde2c3 | ghcr.io/rmi/stitch-entity-linkage:pr-0259 |
ghcr.io/rmi/stitch-entity-linkage:pr-0259@sha256:34bc444baa61d180271843bd4cfa2fac766784ec93f05bf8bf287bcb058c948e |
| 2026-09-04T13:17:34Z | 2026-09-04T13:17:13Z | bdde2c3 | ghcr.io/rmi/stitch-seed:pr-0259 |
ghcr.io/rmi/stitch-seed:pr-0259@sha256:7576c09da4861ffe7feb11e8c18c37680f96fdf40b804f413967a6ae80e9a1ec |
| 2026-09-04T13:17:30Z | 2026-09-04T13:17:13Z | bdde2c3 | ghcr.io/rmi/stitch-stitch-llm:pr-0259 |
ghcr.io/rmi/stitch-stitch-llm:pr-0259@sha256:6bf247b1041f1047502a58f198faa2706cbb02e9ac92ac14e25514b3dd003cb0 |
mbarlow12
left a comment
mbarlow12
left a comment
There was a problem hiding this comment.
Nothing here is badly executed. The problem is that these changes (and somewhat those in #258) work around existing design issues that should be addressed. I'm concerned merging it further reinforces those designs and will make future change harder.
1. AC discrepancy
STIT-418 AC 3: "Same for any relevant Merge Candidates in the Merge Review pane."
The AC asks merge candidates to re-route repointed resources, but these changes appear to only add metadata to the response to inform the reviewer of repointing.
A narrower reading of the AC affords a much smaller change. When approving (A,B) → D, update any pending candidate containing B to contain D instead:
- (B,C) becomes (D,C), stays PENDING, reviewer continues
- no new API field, no frontend util, no CTE change
- ~15 lines in
approve_merge_candidate, same transaction
This might also warrant dropping the fingerprint in favor of a constraint that
a given resource id can't participate in more than one candidate at a time.
2. Additional code churn
Across #258 + #259, we've added ~1000 lines and two new permanent public API fields.
_parent_tree_ctewas stable for months; it's now rewritten to carry an origin
column that serves one view fieldgetis largely duplicated inget_resolvedrequested_resource_id(#258) has zero frontend consumers.ResourceDetailPagereadslocation.state.redirectedFrom, which it sets itselfrepointed_resources(#259) is a second public field for the same concept
Given how we do matching right now, have we actually observed overlapping ids in merge candidates? It seems like the edge case we're handling here is really, really unlikely.
3. Benefits of halting and addressing tech debt
I think it's worth making a decision on STIT-489: all API endpoints live behind stitch-api, keeping in mind that nothing has to move at once.
The point of committing to the direction is that it decouples the API contract from the implementation. Once an endpoint sits behind stitch-api, whether the work happens in a module, a proxied service, or something else is an implementation detail we can revisit later without affecting a consumer. This makes decisions that affect backend internals more reversible and flexible, and it's what makes the two changes below safe to pursue incrementally.
Change 1: one pending candidate per resource. A partial unique index on
merge_candidate_items.resource_id where the candidate is pending. If two pending candidates cannot share a resource, then staleness becomes impossible. I'm also pretty sure this removes the need for fingerprint as well.
Change 2: entity-linkage becomes a module rather than a deployment. Matching moves into stitch-api and excludes repointed resources and resources already claimed by a pending candidate. This would retain the ~220 lines of matching logic and drop essentially everything else from entity-linkage.
This also narrows matching to a single function with a stable signature, roughly propose_merge_groups(session, available_ids) -> list[set[resource_id], ...]. It's in-process with direct DB access, and we'd be free to update the implementation however we want as long as the signature is maintained.
Another way of thinking of this relates to STIT-672 (manual merge candidate creation): a "create merge candidates" form with linkage as an auto-fill feature (like AI enrichment). Proposals are ephemeral and generated on the fly, and we only persist whatever the user actually submits.
Change 3: making the API more strictly RESTful (somewhat out of scope here). This would help clarify the order of operations in this workflow. Candidates become a resource (in the REST sense) with the desired CRUD operations. Today
entity-linkagecreates and persists candidatesapproveboth transitions the candidate and creates a resource- merging is unreachable except through a candidate.
A more RESTful pattern would be:
/og-merge-candidates: GET, POST/og-merge-candidates/{id}: GET (optionally PUT & DELETE)/og-merge-candidates/proposals: what "entity-linkage" becomes- ephemeral GET request that essentially pre-populates a form
- clicking "Submit" is what persists the merge candidates (i.e. POST
/og-merge-candidates)
/oil-gas-fields/merge: approving a merge candidate sends a POST, guarded by checking the merge candidate table- could also be a POST to
/oil-gas-fieldswith the constituent ids
- could also be a POST to
This is a bigger conversation than this PR, but the point is that
- API consolidation allows us freedom to make such decisions
- stricter REST design makes it easier to reason about what should happen for a given operation.
Related open tickets:
| STIT-489 | Consolidate API routes in stitch-api |
| STIT-757 | 15-hour bulk run on prod volumes |
| STIT-644 | Entity Linkage slows down entire app |
| STIT-619 | Speed up the bulk pass (concurrency + read cache) |
| STIT-719 | Bulk run aborts on one transient HTTP timeout |
| STIT-621 | Durable, multi-worker job state |
| STIT-740 | Status stuck "Running" with no progress |
| STIT-620 | Bound the bulk-run result summary |
Setting STIT-489 aside, most of these exist because of the network boundary and in-memory job manager and how the current design/topology constrains our options. Many become either irrelevant or at least much easier to solve with direct DB access and focusing on application boundaries first.
Extends #258 to address the merge-review dead end: when two pending merge
candidates share a source resource, approving one merges that resource away
(its original becomes an all-null shell), leaving the other candidate
functionally invalid. This surfaces that state — hiding the approve/deny
controls and telling the reviewer which resource was merged into which.
Added as a separate PR (stack) primarily because this one has some choices around the ergonomics of merge review that we may want to discuss more without blocking #258
The primary problem I have with the current flow is around the null-shell behavior. I think the best option if we have candidates:
Is that A+C is marked as invalid/outdated, and the merge candidate is taken out of the review queue (like "denied", terminal state, can't be suggested again, but is a 3rd state), and we run the linker on
Din hopes that it picks up a new candidate forC(this should probably be part of the approval, or even the resource creation, process - putting the new resource in the queue to look for matches), so that we would get something like:STIT-418 (part 2 of 2) — resolve repointed resources in the Merge Review pane
Stacked on #258 (base branch
feat/redirect-detail); review/merge part 1first, then this retargets to
main.When an overlapping candidate is approved, a member of another pending candidate
gets merged away. Previously that candidate's Approve was guaranteed to 400 with
a message interpolating
repr()on ORM objects (Repointed: [<...object at 0x...>]), and the only exit was Deny — recording a judgment no one made.repointed_resources(which members movedand their terminal resource), computed for PENDING candidates via a single
batched root lookup. The approve-time guard now names the terminal resource
("resource 102 is now resource 301") on both the merge-candidate and
write-path guards.
hides Approve/Deny when a member has moved, and annotates moved members with
"(now 301)".
Also includes a small perf commit dropping redundant queries on the resolve
paths (
get_resolvedbuilds the entity from the row it already loaded;createskips an always-empty staleness lookup).
The retire/requeue/
SUPERSEDED-status flow is intentionally deferred until thestale-candidate dead end actually blocks a reviewer.
Tests
Backend: origin-column batch mapping, N+1 batch guard, PENDING-only rule, the
cross-candidate case end-to-end (approve 400s naming both ids, no
object at 0x, stays PENDING), and the apply_resource_merge guard case. Frontend: stalebanner + hidden controls, facts annotation, compat guard, and a pure-function
suite for the staleness util.
AI assistance
Implemented with Claude Code. Verified: API pytest + frontend vitest green.