feat(review): actionable Project identity repair and honest stale-source grouping - #1545
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6279c63450
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target.workspace_identity, | ||
| projection.project.canonicalIdentity, | ||
| sourceScopeId, |
There was a problem hiding this comment.
When the source has memories in multiple Spaces, the selected sourceScopeId is stored on a single mapping, but the alias is keyed only by projection.project.canonicalIdentity; projection collapse therefore redirects every session for that source, including memories from the unselected Spaces. The repaired target consequently retains multiple scope boundaries and immediately becomes an ambiguous_scope_mapping finding even though this function returns applied. Either migrate only the selected Space's records or reject this repair shape instead of reporting success.
Useful? React with 👍 / 👎.
| "DELETE FROM recipient_policy_authority_states WHERE canonical_project_identity = ?", | ||
| ).run(item.canonicalProjectIdentity); | ||
| const deleted = db | ||
| .prepare("DELETE FROM project_recipients WHERE canonical_project_identity = ?") |
There was a problem hiding this comment.
When a stale identity contains a user-managed or invite-derived recipient row alongside legacy state, this unqualified delete removes every recipient assignment for the identity. Other write paths explicitly distinguish these rows through provenance and migration_state, while the confirmation promises to remove only inert legacy sharing records. Restrict both eligibility and deletion to the legacy-owned rows so cleanup cannot erase current policy configuration.
Useful? React with 👍 / 👎.
| removableCount: staleGroup.items.filter( | ||
| (item) => legacySharingRowsForIdentity(db, item.canonicalProjectIdentity).length > 0, | ||
| ).length, |
There was a problem hiding this comment.
When one stale source has multiple project_recipients rows, removableCount increments once for the identity rather than once per deletable row. The UI nevertheless labels this value as a record count, and the prune result similarly reports one removed item even though the SQL deletes several rows, so the confirmation and completion notice underreport the destructive operation. Count the matching rows or label these values as source counts.
Useful? React with 👍 / 👎.
|
Closing as superseded by the safer recipient-policy repair flow in #1561. The remaining persisted alias-mapping and stale-recipient cleanup work is not safe to merge from this stale branch: review identified multi-Space alias scope leakage, deletion of non-legacy recipient rows, and inaccurate cleanup counts. Any still-needed mapping or cleanup behavior should return as focused follow-up PRs against current main with those cases designed out. |
1 participant
Description
The recipient-policy review told users a Project needed repair and then gave them nothing usable. This makes those findings actionable, honest, or explicitly inert.
Before: blocked findings rendered a disabled
Choose Projectcontrol and the message "No safe canonical Project choices are available for this source." Every one of the 5 live findings was a dead end. An earlier iteration shipped aFix Project identitycontrol that only printedgit remote addinstructions and reloaded — adding a remote cannot rewrite historical records, so it repaired nothing.After:
Choose Project → Save mappingwrites aproject_scope_mappingsalias from the legacy source identity to a canonical Project, then refreshes the finding away. Reuses the Team setup picker pattern. Fail-closed on stale fingerprint, invalid/inactive target scope, foreign or conflicting mapping, and on a higher-priority wildcard that would keep the inserted row non-authoritative (matchingexplicitResolutionForprecedence, so a "success" cannot silently lose to a wildcard).removableCountreports how many own deletableproject_recipientsrows; when it is 0 the cleanup control is not rendered at all (a hidden "Remove 0 records" still reaches assistive tech).pruneStaleRecipientPolicySourcesdeletes only inert legacy sharing rows, in one transaction, re-deriving every guard server-side, with a group fingerprint guard and per-item skip reasons. Never touches memories, sessions, or Spaces.multiple_enforcement_boundariesnow names the conflicting Spaces and offers Review Space mappings, which opens Advanced Project administration and focuses that Project's Space select.Also fixes, all found while verifying against live data:
detailspanel deferred every render, and a focused Space select madeloadProjectsDatareturn before fetching. User-initiated loads (search, status filter, pagination) now always run and always paint; background refreshes keep the anti-jump behavior.shared:defaultwas labelled "Local folder (path hidden)" because its:fails the path-safety pattern. It is a synthetic compatibility bucket, now shown as Shared (default).Assign to Project…directly on path cards, and replaced repeated genericLegacy Teamlabels with the coordinator Team name.Reviewer note on one rejected approach
An intermediate version classified any zero-memory Project holding a sharing row as stale. On real data that swept in ~20 canonical
greenroomworktree Projects — all mislabelledgreenroom— and offered to delete their recipient rows. Staleness is now decided only by the narrownoncanonical_project_identityrepair path. Worth a close look during review.Type of Change
Testing
pnpm run tsc,pnpm run lint,pnpm run test)Full suite green: 222 files, 5558 tests. Verified against the live viewer on a real database:
removableCount: 0, no cleanup button, no raw local paths in the payload.Checklist
pnpm run lintpasses for touched files)