fix(architecture): enforce Managed System compatibility for canonical entity links - #425
fix(architecture): enforce Managed System compatibility for canonical entity links#425hjung3113 wants to merge 2 commits into
Conversation
… entity links Adds assertLinkManagedSystemCompatibility as the single create-time gate rejecting cross-MS entity links (validation.failed/managed_system_mismatch) before any capability check, insert, or audit row — the ordering matches docs/implementation/06-entity-linking-contract.md's already-documented validation order. Wired into createLink (generic POST /entity-links, Finding link-evidence) plus the domain paths that insert links directly: findings.createFindingFromVoc and voc-clusters.createFindingFromCluster/ linkExistingFinding. Reconciles the two integration tests that relied on cross-MS creation succeeding as an unrelated hidden-stub-visibility fixture: they now seed the cross-MS row directly instead of via POST. Adds AC-named rejection tests for the generic path, all registered same-MS pairs (regression), and the Finding/VOC-Cluster source-derived paths, each asserting on DB row/audit counts rather than just HTTP status. DB-level enforcement was investigated and rejected: core.entity_links is polymorphic (source_type/target_type pick the owning table), so a CHECK/FK can't express the compatibility rule without per-type triggers duplicating application authorization in SQL — documented in the contract and this module's AGENTS.md, which also had its registeredEntityLinkPairs count fixed (stale at 10, actually 14). Verified end-to-end against a throwaway Postgres: full backend integration suite is 1269/1270 green, the one failure is the pre-existing #419 (list-vocs tab=high, unrelated, confirmed via git stash). Closes #388. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5bvJ64nqUZWP8bDkpqJGk
![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: 9f9edac25c
ℹ️ 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".
| * Create-time Managed System compatibility gate (#388). Both endpoints of an | ||
| * entity link must resolve to the same Managed System; a mismatch is rejected | ||
| * before any capability check or insert. This is the single authoritative seam | ||
| * — the polymorphic entity_links table cannot express it as a CHECK/FK. |
There was a problem hiding this comment.
When a pre-existing cross-MS VOC→Finding evidence link is converted through a Finding's Task Request, preserveSourceLinks in tasks/service.ts:139-165 copies it into a new VOC→Task link without resolving the VOC's Managed System or invoking this gate. Such rows can exist because cross-MS creation was previously allowed and this commit deliberately retains them for reads, so Task conversion can continue creating the canonical cross-MS links this change is intended to prohibit. Validate or reconcile inherited evidence before inserting the propagated link.
AGENTS.md reference: apps/backend/src/modules/entity-links/AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
맞는 지적입니다. tasks/service.ts preserveSourceLinks에 assertLinkManagedSystemCompatibility 호출 추가(41b6a39) — legacy cross-MS finding→task_request, voc→finding evidence 링크 전파 전에 검증. 스로어웨이 DB로 tasks suite 42/42 확인.
| throw new HttpError('not_found.record', 'entity link endpoint not found'); | ||
| } | ||
|
|
||
| assertLinkManagedSystemCompatibility(sourceRow.managed_system_id, targetRow.managed_system_id); |
There was a problem hiding this comment.
On POST /findings/:id/link-evidence, a Developer who can manage a Finding in MS A but cannot read a supplied VOC in MS B now receives 422 managed_system_mismatch, while a missing or same-MS unreadable VOC receives the non-disclosing 404 from findings/service.ts:859-863. Because this check runs before sourceProvider.canRead, it exposes the existence and cross-MS placement of a hidden VOC; run compatibility validation only after endpoint authorization succeeds, while still keeping it before persistence.
AGENTS.md reference: apps/backend/src/modules/entity-links/AGENTS.md:L3-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
맞는 지적입니다. 41b6a39에서 MS-compat 체크를 sourceProvider.canRead/targetProvider.canRead(canCreateTarget) 통과 후, persist 직전으로 이동했습니다. findings.linkEvidence의 permission.denied→404 매핑이 다시 정상 작동합니다(unreadable+cross-MS VOC → 여전히 404). evidence-highlights 테스트도 그 케이스를 명시적으로 커버하도록 갱신.
| ); | ||
| // The seeded cross-MS row from the first test still exists; the HTTP | ||
| // create path must not have added or audited a second one. | ||
| expect(persisted.rows[0]?.links).toBe('1'); |
There was a problem hiding this comment.
When this AC is run through Vitest's supported narrow filtering, the earlier test that seeds the cross-MS row is skipped, so links is 0 and this assertion fails despite correct production behavior. The test is therefore order-dependent; create its prerequisite row in this test or compare counts before and after the request instead.
AGENTS.md reference: apps/backend/AGENTS.md:L38-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
맞는 지적입니다. 41b6a39에서 절대 카운트 대신 before/after delta로 변경했습니다. 이 테스트만 단독 필터링해서(다른 seeding 테스트 스킵된 상태로) green 확인했습니다 — order-independence 검증 완료.
Addresses 3 automated review findings on PR #425: 1. assertLinkManagedSystemCompatibility ran before endpoint authorization in createLink, so an actor without read access to a source VOC could learn it exists and is cross-MS via 422 instead of the disclosure-safe 404 findings.linkEvidence maps permission.denied to. Moved the check to run after both source/target authorization succeed, before persist. 2. preserveSourceLinks (tasks/service.ts) propagated legacy pre-#388 finding->task_request and voc->finding evidence links into new finding->task and voc->task links during Task Request conversion without checking Managed System compatibility, defeating the new gate for grandfathered cross-MS data. Now asserts compatibility before each propagated insert, consistent with every other write path in this PR. 3 AC-388-4 in link-existing-finding.integration.test.ts asserted an absolute row count that only held because an earlier test in the same file had already seeded a cross-MS fixture row as a side effect — order-dependent, fails under narrow test filtering. Switched to a before/after row-count delta, verified independently by running the test alone. Verified against a throwaway Postgres: entity-links/findings/voc-clusters/ tasks suites all green, AC-388-4 passes in isolation (confirms order- independence), full backend suite 1269/1270 (only pre-existing #419 fails, unrelated). typecheck/boundaries/biome clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5bvJ64nqUZWP8bDkpqJGk
1 participant
Summary
assertLinkManagedSystemCompatibility(entity-links/service.ts) is the single create-time gate rejecting cross-MS entity links, matching the validation order already documented in06-entity-linking-contract.md.POST /entity-links, Finding link-evidence, and the domain paths that insert links directly:createFindingFromVoc,createFindingFromCluster,linkExistingFinding.core.entity_linksis polymorphic, so application enforcement is authoritative (documented in the contract + module AGENTS.md, which also had its stale tuple count fixed 10→14).Test plan
pnpm --filter backend test:integrationagainst a throwaway Postgres — 1269/1270 green (only failure is pre-existing fix(voc): tab=high leaks critical-severity VOCs from unscoped GET query #419, unrelated, confirmed via git stash)pnpm typecheck— 7/7pnpm check:boundaries— OKbiome checkon all touched files — cleanCloses #388
🤖 Generated with Claude Code
https://claude.ai/code/session_01H5bvJ64nqUZWP8bDkpqJGk