feat(ontology): search assertion-backed occupational constructs - #755
Conversation
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bf13490 to
b702906
Compare
|
Rebased onto current main |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
21516ab to
1ac5a4c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| construct.preferred_label ilike $1 escape E'\\' | ||
| or coalesce(construct.construct_description, '') ilike $1 escape E'\\' |
There was a problem hiding this comment.
🔴 Catalog search always errors against a real database
The ilike $1 escape E'\\' clauses live in a non-raw Python string, so the SQL actually sent is escape E'\'. In a Postgres escape string that backslash escapes the closing quote, leaving the literal unterminated, so search_visible_occupational_constructs raises a syntax error on every real query. The mock-connection tests only check for the substring ilike $1 escape E, so they miss it.
| construct.preferred_label ilike $1 escape E'\\' | |
| or coalesce(construct.construct_description, '') ilike $1 escape E'\\' | |
| construct.preferred_label ilike $1 escape E'\\\\' | |
| or coalesce(construct.construct_description, '') ilike $1 escape E'\\\\' |
Was this helpful? React with 👍 or 👎 to provide feedback.
| hits, extra_visible = _collapse_visible_hits(rows, can_see_post, limit=page_size) | ||
| candidate_constructs = { | ||
| str(_row_mapping(row)["construct_iri"]) for row in rows | ||
| } | ||
| sql_exhausted = len(candidate_constructs) == CANDIDATE_CONSTRUCT_LIMIT | ||
| next_cursor = None | ||
| if extra_visible and hits: | ||
| next_cursor = hits[-1].construct_iri | ||
| elif sql_exhausted and rows: | ||
| next_cursor = str(_row_mapping(rows[-1])["construct_iri"]) | ||
| return OccupationalConstructSearchPage( | ||
| query=normalized_query, | ||
| family_code=normalized_family, | ||
| hits=tuple(hits), | ||
| next_cursor=next_cursor, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Keyset pagination holds across visibility and candidate-window cuts
The candidate window (dense_rank capped at 200, ordered by construct_iri) and _collapse_visible_hits agree on IRI order; the break at limit+1 plus a strict construct_iri > $3 cursor prevents skips and duplicates. The sql_exhausted fallback (distinct == 200) correctly continues when visibility filtering thins a page, and distinct < 200 proves no further candidates exist.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for construct_id in order: | ||
| visible_rows = grouped[construct_id] | ||
| if int(visible_rows[0]["construct_row_count"]) > PER_CONSTRUCT_ROW_LIMIT: | ||
| continue | ||
| truth_statuses = {str(item["truth_status_code"]) for item in visible_rows} | ||
| if len(truth_statuses) != 1: | ||
| continue | ||
| truth_status = next(iter(truth_statuses)) | ||
| if truth_status in WITHDRAWN_TRUTH_STATUSES: | ||
| continue |
There was a problem hiding this comment.
📝 Info: Constructs with >200 supporting rows never return a hit
_collapse_visible_hits skips any construct whose partition count exceeds PER_CONSTRUCT_ROW_LIMIT before the truth-conflict check, which correctly fails closed on a truncated group. A side effect: a legitimately broad construct with over 200 visible, agreeing supporting posts also returns nothing. Appears intentional, but it silently limits recall for common labels.
Was this helpful? React with 👍 or 👎 to provide feedback.
| where ( | ||
| construct.preferred_label ilike $1 escape E'\\' | ||
| or coalesce(construct.construct_description, '') ilike $1 escape E'\\' | ||
| ) |
There was a problem hiding this comment.
📝 Info: Description-only matches show a label without the search term
The query filters both preferred_label and construct_description, but the payload carries only the label. A hit matched solely on the undisclosed description shows a label lacking the search term. Consistent with ADR 0257 (match either, disclose neither description), noted for reviewer UX awareness.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async function onMore() { | ||
| if (!accessToken || !page?.next_cursor) return; | ||
| setStatus("loading"); | ||
| try { | ||
| const result = await fetchOccupationalConstructSearch(accessToken, { | ||
| query: page.query, | ||
| family: page.family_code || undefined, | ||
| knowledgeCutoff, | ||
| cursor: page.next_cursor, | ||
| }); | ||
| setPage({ ...result, hits: [...page.hits, ...result.hits] }); | ||
| setStatus("ready"); | ||
| } catch { | ||
| setStatus("error"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Show-more loading hides the current results
In onMore the status is set to loading while paginating, and the list only renders when status is ready, so the already-shown hits vanish and are replaced by the loading line until the fetch resolves. Functionally correct, minor flicker.
Was this helpful? React with 👍 or 👎 to provide feedback.
Authorized reviewers can type an official O*NET catalog label in the ontology explorer and open the earliest visible supporting record (ADR 0256). Hidden posts, withdrawn truth, and catalog rows without visible evidence stay omitted. No scores or person traits.
The rebased branch references ADR 0256 for catalog-bound search; merge that reference into main's current rule index.
da9f9f6 to
855a640
Compare
855a640 to
9486231
Compare
| sql_exhausted = len(candidate_constructs) == CANDIDATE_CONSTRUCT_LIMIT | ||
| next_cursor = None | ||
| if extra_visible and hits: | ||
| next_cursor = hits[-1].construct_iri | ||
| elif sql_exhausted and rows: | ||
| next_cursor = str(_row_mapping(rows[-1])["construct_iri"]) |
There was a problem hiding this comment.
📝 Info: next_cursor can emit one empty extra page at exactly 200 constructs
In search_visible_occupational_constructs, sql_exhausted becomes true whenever exactly 200 distinct constructs match, even when none remain beyond them. next_cursor is then set, so the follow-up request returns an empty page. Benign: keyset continuation is strict and drops no hits.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _collapse_visible_hits( | ||
| rows: list[Any], | ||
| can_see_post: Callable[[Any], bool], | ||
| *, | ||
| limit: int, | ||
| ) -> tuple[list[OccupationalConstructSearchHit], bool]: | ||
| """Keep one earliest visible Post per construct; drop conflicts and withdrawn truth.""" | ||
| grouped: dict[str, list[Mapping[str, Any]]] = {} | ||
| order: list[str] = [] | ||
| for row in rows: | ||
| if not can_see_post(row): | ||
| continue | ||
| mapping = _row_mapping(row) | ||
| construct_id = str(mapping["construct_id"]) | ||
| if construct_id not in grouped: | ||
| grouped[construct_id] = [] | ||
| order.append(construct_id) | ||
| grouped[construct_id].append(mapping) | ||
|
|
||
| hits: list[OccupationalConstructSearchHit] = [] | ||
| for construct_id in order: | ||
| visible_rows = grouped[construct_id] | ||
| if int(visible_rows[0]["construct_row_count"]) > PER_CONSTRUCT_ROW_LIMIT: | ||
| continue | ||
| truth_statuses = {str(item["truth_status_code"]) for item in visible_rows} | ||
| if len(truth_statuses) != 1: | ||
| continue | ||
| truth_status = next(iter(truth_statuses)) | ||
| if truth_status in WITHDRAWN_TRUTH_STATUSES: | ||
| continue | ||
| chosen = min( | ||
| visible_rows, | ||
| key=lambda item: (item["available_at"], str(item["post_id"])), | ||
| ) | ||
| hits.append( | ||
| OccupationalConstructSearchHit( | ||
| construct_id=str(chosen["construct_id"]), | ||
| construct_iri=str(chosen["construct_iri"]), | ||
| construct_family_code=str(chosen["construct_family_code"]), | ||
| preferred_label=str(chosen["preferred_label"]), | ||
| vocabulary_version=str(chosen["version_label"]), | ||
| supporting_post_id=str(chosen["post_id"]), | ||
| supporting_post_title=str(chosen["post_title"]), | ||
| evidence_text=str(chosen["evidence_text"]), | ||
| truth_status_code=truth_status, | ||
| ) | ||
| ) | ||
| if len(hits) == limit + 1: | ||
| break | ||
| truncated = len(hits) > limit | ||
| return hits[:limit], truncated |
There was a problem hiding this comment.
📝 Info: Hidden posts can suppress a visible construct via the row-count cap
In _collapse_visible_hits, a construct is dropped when construct_row_count exceeds 200. That count is computed in SQL before the ABAC filter, so hidden posts inflate it. Enough hidden support can make an otherwise-visible construct vanish, in tension with the ADR promise that hidden Posts never change visible results. Requires >200 assertions on one construct; likely an intentional fail-closed bound.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const meta = { | ||
| title: "Evidence/OccupationalConstructCatalogSearch", | ||
| component: OccupationalConstructCatalogSearch, | ||
| } satisfies Meta<typeof OccupationalConstructCatalogSearch>; |
There was a problem hiding this comment.
🟡 New Storybook story absent from the required inventory
AGENTS.md requires every new story to be listed in the story inventory. The added Evidence/OccupationalConstructCatalogSearch story has no entry there.
Prompt for agents
AGENTS.md states new stories belong in the inventory at docs/storybook-inventory.md. Add a row describing the new Evidence/OccupationalConstructCatalogSearch story (its scenes: Idle, Populated, NoMatches, Loading, Unavailable, NarrowViewport) and the tokens/components it exercises, matching the format of existing inventory rows.
Was this helpful? React with 👍 or 👎 to provide feedback.
The merge-rebase of the occupational-construct search feature dropped the closing brace of fetchWorkerFunctionConstructCatalog and ate the 'e' of 'export' when splicing the branch's search client block. Restore both; oxlint, tsc, and the 453-test suite are green.
| and construct.construct_family_code in ( | ||
| 'cognitive_ability', 'work_style', 'work_activity' | ||
| ) | ||
| and ($3::text is null or construct.construct_iri > $3) |
There was a problem hiding this comment.
🔍 Keyset on construct_iri can skip a same-IRI versioned construct
The cursor and dense_rank order on construct_iri, but node identity is the versioned construct_id. If two vocabulary versions share one external IRI, the strict construct.construct_iri > $3 next-page filter drops the second construct when a page boundary lands on that IRI. Unreachable with a single synchronized O*NET 31.0 catalog, but the schema permits versions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| and job.source_body_sha256 = extraction.source_body_sha256 | ||
| where ( | ||
| construct.preferred_label ilike $1 escape E'\\' | ||
| or coalesce(construct.construct_description, '') ilike $1 escape E'\\' |
There was a problem hiding this comment.
📝 Info: Description trigram index unused by coalesce predicate
The description branch filters coalesce(construct_description,'') ilike $1, while the new GIN trigram index is on the bare column. Postgres cannot use the index through the coalesce wrapper, so the description match falls back to a scan. Correctness is unaffected since NULL never matches a %term% pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
| elif sql_exhausted and rows: | ||
| next_cursor = str(_row_mapping(rows[-1])["construct_iri"]) |
There was a problem hiding this comment.
🟨 Continuation cursor discloses hidden catalog membership
The sql_exhausted fallback fills next_cursor from a row whose supporting record is hidden from the caller. An empty page then carries a cursor when at least 200 constructs matched but all support is hidden, while a genuinely unmatched query carries none, disclosing catalog membership the reviewer has no authorized evidence for.
Was this helpful? React with 👍 or 👎 to provide feedback.
ADR 0257 was claimed by the promoted O*NET occupation-rating observation store (#759); the search branch's catalog-search ADR collided. Renumber to 0265 across the file, README map, and ADR 0255 reference, restoring the unique-ADR-number hygiene gate.
| {status === "ready" && page && page.hits.length > 0 ? ( | ||
| <> | ||
| <ul className="ticket-list" aria-labelledby="occupational-construct-catalog-search-heading"> | ||
| {page.hits.map((hit) => ( | ||
| <CatalogHitItem key={hit.construct_id} hit={hit} onSelectPost={onSelectPost} /> | ||
| ))} | ||
| </ul> | ||
| {page.next_cursor ? ( | ||
| <button type="button" onClick={onMore}>{text("Show more matching records")}</button> | ||
| ) : null} | ||
| </> | ||
| ) : null} |
There was a problem hiding this comment.
🟡 Empty search page with a cursor cannot be continued
The backend can return zero hits together with a continuation cursor (the exhausted-candidate branch of search_visible_occupational_constructs). The Show more matching records button renders only in the ready state with hits, so the reviewer sees "no matches" and cannot page to later constructs that do have visible support.
Prompt for agents
search_visible_occupational_constructs in backend/app/occupational_construct_search.py can return an empty hits list together with a non-null next_cursor (its sql_exhausted branch, used when the first 200 candidate constructs are all filtered out by ABAC/conflict/withdrawn). In OccupationalConstructCatalogSearch.tsx, the 'Show more matching records' button is only rendered inside the block gated on status === 'ready' && page.hits.length > 0. As a result, an empty page that still carries next_cursor becomes a dead end: the reviewer sees the 'No visible work evidence matches' empty state and cannot continue paging to later constructs that may have visible support. Consider surfacing the continuation control (or auto-continuing) whenever page.next_cursor is set, including in the empty state, so a truncated first batch does not present as a definitive no-match.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -0,0 +1,302 @@ | |||
| """Search assertion-backed occupational constructs under ADR 0257.""" | |||
There was a problem hiding this comment.
🟡 Catalog search code cites the wrong ADR
The module is attributed to ADR 0257, but that number belongs to the accepted O*NET occupation-rating observation store; this feature is governed by ADR 0265. The citation points to an unrelated decision.
| """Search assertion-backed occupational constructs under ADR 0257.""" | |
| """Search assertion-backed occupational constructs under ADR 0265.""" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -0,0 +1,11 @@ | |||
| -- ADR 0257: index assertion-backed catalog labels for authorized search. | |||
There was a problem hiding this comment.
🟡 Search migration cites the wrong ADR
The comment attributes these indexes to ADR 0257, which is the accepted occupation-rating observation store. The governing decision for catalog search is ADR 0265.
| -- ADR 0257: index assertion-backed catalog labels for authorized search. | |
| -- ADR 0265: index assertion-backed catalog labels for authorized search. |
Was this helpful? React with 👍 or 👎 to provide feedback.
| Authorized occupational construct catalog search (ADR 0257) matches official | ||
| O*NET preferred labels or descriptions only when a source-eligible, ABAC-visible | ||
| Post supports that construct. Hidden Posts, withdrawn truth, and conflicting | ||
| truth statuses omit the hit. Clicking a hit opens that Post. Do not return | ||
| catalog rows as a vocabulary oracle, scores, or person traits. Continuation | ||
| is a construct-IRI keyset; never OFFSET. |
There was a problem hiding this comment.
🟡 AGENTS.md cites the wrong ADR for catalog search
This paragraph attributes catalog search to ADR 0257, which is the accepted occupation-rating observation store. The governing decision is ADR 0265.
Was this helpful? React with 👍 or 👎 to provide feedback.
| (ADR 0048–0164 / 0182 / 0185 / 0201 / 0233), occupational construct catalog search | ||
| (ADR 0257), the text-channel embedding swap and cosine |
There was a problem hiding this comment.
🟡 CLAUDE.md cites the wrong ADR for catalog search
The pointer attributes occupational construct catalog search to ADR 0257, the accepted occupation-rating observation store. The feature is ADR 0265.
| (ADR 0048–0164 / 0182 / 0185 / 0201 / 0233), occupational construct catalog search | |
| (ADR 0257), the text-channel embedding swap and cosine | |
| (ADR 0048–0164 / 0182 / 0185 / 0201 / 0233), occupational construct catalog search | |
| (ADR 0265), the text-channel embedding swap and cosine |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Authorized reviewers can type an official O*NET catalog label in the ontology explorer and open the earliest visible supporting record (ADR 0256 / v2.20.0).
After seed and construct extraction, Find work evidence sits on the existing ontology explorer (not a new GNB destination). Two or more letters of a catalog label, then Find matching records, lists assertion-backed hits. Click opens that post.
truth_rejectedandtruth_supersededdo not hit.cognitive_ability,work_style, andwork_activity.Does not mix into #750 / #640 / #680 / #720. Issues #79 / #87 stay open.
Test plan
pytest tests/test_occupational_construct_search.py tests/test_occupational_construct_catalog_schema.py(9 passed)Independent APPROVE required. Do not self-approve.