feat(users): resolve users by name, email or username wherever a user UUID was required

[betty-pr-factory-app]

🎯 What does this PR do?

The assessment tools (create_assessment, edit_assessment) demanded a user UUID and
nothing else, while edit_asset's responsibility operations have accepted a UUID, an email
or a username since they shipped. This gives every user-valued parameter the same latitude,
extends the shared resolver to cover a person's display name, and adds a small read-only
tool that returns a user's UUID from any of those forms.

The defect being fixed. search_asset_keyword already resolves five of its six filters
from names through pickMatch, which returns one match, a self-correcting "not found" with
suggestions, or an ambiguity error listing every candidate with its id. Users never reached
it: the lookup called FindUserByUsername, which collapsed to a single exact-username match
before a second candidate could exist. A display name therefore resolved to nothing.

What changed

• New pkg/tools/resolve package. pickMatch, namedRef and suggestionSuffix move
  out of search_asset_keyword into a shared package, following the pkg/tools/validation
  precedent. search_asset_keyword imports them back for all six of its filters — that is
  why the diff is wide, but it is a move, not a rewrite: the matching, the message
  formats and the three outcomes are unchanged. The one addition is resolve.Hints, which
  replaces the old param argument so a caller with no filter parameter can word its own
  "how to disambiguate" instruction.
• clients.FindUsersByName returns EVERY enabled user matching a name instead of one,
  and sends nameSearchFields (USERNAME, FIRSTNAME, LASTNAME, FIRSTNAME_LASTNAME,
  LASTNAME_FIRSTNAME) and includeDisabled=false explicitly rather than relying on the
  server defaults. FindUserByUsername is removed: every caller now goes through the shared
  resolver or the notification-recipient helper, both of which need the full candidate list
  from a single request. The exact-username scan it existed for remains as
  exactUsernameMatch.
• Resolution precedence is exactly what it was, with one new step at the end: a UUID
  passes through with no request issued, a value containing @ goes to the exact email
  endpoint, an exact username wins next, and only then does the value fall through to
  display-name candidates. A candidate is presented as First Last with its username as
  disambiguating context and its UUID. Email addresses never appear in output — no CHIP
  tool returns one today and this change does not start.
• Ambiguity stays an ERROR carrying the candidate list, as pickMatch does today, not a
  success payload with a candidates array. The wording is new for a lookup with no filter
  parameter: "do NOT pick one — ask which person is meant, then call again with that user's
  username or UUID". This matters because get_user_id_by_name runs mid-chain to fill
  another call's parameter, and a model holding a list of plausible ids and a pending call
  will pick one.
• New get_user_id_by_name tool (read-only): full name, username, email address or
  UUID in, UUID plus full name and username out.
• Consumers updated: create_assessment (owner, assignees), edit_assessment
  (set_owner, set_assignees), edit_asset (set_responsibility,
  remove_responsibility), search_asset_keyword (createdByFilter) and data quality
  notification recipients. A GROUP assignee still requires a UUID — group names are not
  resolvable through the user lookup.
• Test-double correction. The /rest/2.0/users double in pkg/tools/edit_asset/tool_test.go
  matched a three-field OR over username, first name and last name. Under it no two-word name
  could ever match, so it encoded an API that does not exist and a test written against it
  would have passed while asserting the opposite of real behaviour. It now mirrors the
  endpoint's nameSearchFields, including both concatenated forms. The comment on the client
  function was incomplete in the same way and is corrected.

Deliberate deviation from the contribution standards. get_user_id_by_name registers
without an experimental feature flag, which departs from
docs/TOOL_CONTRIBUTION_STANDARDS.md §3.1. The assessment tools it supports are themselves
ungated (pkg/tools/register.go), so a flagged resolver behind unflagged consumers is exactly
the broken-half-a-pair failure §1.1 warns about: a caller that advertises "give me a name" with
no tool available to turn one into a UUID. The rationale is repeated as a comment at the
registration site.

---

Impact Analysis

• Backwards compatible. Every form accepted before still resolves the same way, and a
  UUID still costs no lookup. What changes is that more inputs now succeed, plus two error
  messages: an unresolvable user reads no user matching "x" found. … rather than
  no user found matching "x" …, and a shared name is now an ambiguity error rather than a
  not-found. Both remain per-operation errors in edit_asset and abort the atomic PATCH in
  edit_assessment — nothing is written on a failed resolution.
• Request cost is unchanged for UUIDs (no request) and emails (one request). A username
  or display name costs the one /rest/2.0/users search it already cost.
• Wide diff, narrow behaviour change. The bulk of the line count is the helper move and
  the new tests; search_asset_keyword's five other filters are untouched in behaviour.
• Merge conflicts. Two open PRs touch these files — fix(assessments): fixing assessment input schema, which was breaking Gemini Enterprise integration #128 (draft, rewrites the
  assessment tools' input schema) and fix(assessments): fixing assessment output schema #139 (assessment output schema). This work takes
  priority and merges first, so both will need a rebase; the conflicts are in
  create_assessment/tool.go and edit_assessment/tool.go, where owner/assignee validation
  is replaced by resolution.
• Verification. go test ./... and golangci-lint run both pass. go test -race was
  NOT run: the sandbox image has no C compiler and CGO_ENABLED=0, so it exits with
  "-race requires cgo". CONTRIBUTING.md documents -race; it needs a toolchain this
  environment does not have, so CI is the check for it.

---

Checklist

• My code follows the style guidelines of this project.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation (if needed).
• My commit messages follow the Conventional Commits standard.

[betty-pr-factory-app]

Review

Reviewed at 9a9fa26 against main, with docs/TOOL_CONTRIBUTION_STANDARDS.md as the governing document. There were no findings from a previous round to re-judge (.pesto/inputs/review_findings/ was absent), so this is a first pass.

The shape of the change is right: one shared resolver, one precedence chain, every user-valued parameter routed through it, and a wide-but-mechanical move of pickMatch into pkg/tools/resolve following the pkg/tools/validation precedent. The tests are the strong part — they pin the precedence, the ambiguity contract and the "nothing is written" guarantee, and the correction to the /rest/2.0/users double in edit_asset fixes a mock that encoded an API which does not exist.

What I ran here (a constrained sandbox — no repo-wide runs, no -race, no lint):

go build ./...                                                          # clean
go vet ./pkg/tools/resolve/... ./pkg/tools/get_user_id_by_name/... \
       ./pkg/clients/... ./pkg/tools/edit_asset/...                     # clean
go test ./pkg/tools/resolve/... ./pkg/tools/get_user_id_by_name/... \
        ./pkg/tools/create_assessment/... ./pkg/tools/edit_assessment/... # ok
go test ./pkg/tools/edit_asset/... ./pkg/tools/search_asset_keyword/... \
        ./pkg/clients/... ./pkg/tools/                                    # ok
gofmt -l <every touched package>                                        # no output

go test ./..., -race and golangci-lint were not run in this environment; CI is the check for those.

The four decisions, verified

1. get_user_id_by_name registered with no experimental flag (deviation from §3.1). Done as described. pkg/tools/register.go:102-108 registers it outside every IsExperimentalEnabled block with the rationale in a comment citing §3.1 and §1.1, and the consumers it feeds are ungated in the same block. register_test.go:32-39 asserts it is visible with an empty ServerToolConfig. Nothing was added to cmd/chip/experimental.go, which is correct — there is no flag to register. §5.3 annotations are set explicitly.
2. Ambiguity is an error carrying candidates, not a success payload. Done as described. PickMatch returns an error for more than one exact match; get_user_id_by_name returns Output{} plus the error, and Output has no candidates field to tempt a pick. The write paths abort before writing: edit_assessment fails the op and skips the atomic PATCH, edit_asset returns a per-op error and creates no responsibility (asserted by TestEditAsset_SetResponsibility_AmbiguousFullNameWritesNothing). The one exception — notification recipients falling to Unresolved — is appropriate for a best-effort list, but see finding 2 about telling the model which of the two failures it hit.
3. Email in, never out. Done as described for directory data: resolve.User carries only ID/username/full name, toUser drops the address, candidates are labelled username:, and three tests assert no @ in the error text. One boundary worth knowing rather than fixing: edit_assessment echoes the caller's own set_owner input into results[].value, and edit_asset echoes it into results[].userId, so an email the model itself supplied can come back in the payload. That is input echo, not disclosure, and it pre-dates this PR in edit_asset.
4. pickMatch move is a move. Confirmed. Old and new map 1:1 — same normalize-and-compare matching, same three outcomes, byte-identical message formats, same sort and max-15 truncation. The only semantic delta is param becoming resolve.Hints, with filterHints(param) reproducing the old "pass the UUID in <param> to disambiguate" wording verbatim. The other five filters are pure identifier renames, every pre-existing search_asset_keyword test is unchanged in the diff, and they pass.

What should change before merge

1. edit_asset's tool description was left behind (§7.1, §7.3, §6.2). pkg/tools/edit_asset/tool.go:161 still reads "set_responsibility (… the user can be given as a UUID, username, or email)". The field tag, README.md and asset-edit/SKILL.md all learned about full names; the description — the text the model actually reads to choose the tool, and the surface §7 governs — did not.

2. The DQ job tools' notifyRecipients descriptions are now stale (§6.2, §7.1). This PR extended ResolveNotificationRecipients to display names, and create_dq_job/tool.go:353 and update_dq_job/tool.go:465 both call it, but their field tags still say "by username or email" (create_dq_job/tool.go:130, update_dq_job/tool.go:105). Two things go undocumented: full names now work, and an ambiguous full name lands in Unresolved indistinguishably from a typo — so the model is told "unresolved" with nothing to correct against (§6.3). findRecipientByName already knows which case it hit; carrying the reason through would be cheap.

3. A truncated user search can silently bind the wrong person. FindUsersByName sends limit=100 and listUsers discards page.Total (edit_asset_client.go:826-852) — editAssetUsersList parses Total and nobody reads it. Under the old code the only consumer was an exact-username scan and usernames are unique, so truncation could at worst yield a not-found. Now the same truncated window feeds display-name matching: if a substring search matches more than 100 users and exactly one of the first 100 has the exact full name, PickMatch reports a confident single match while a second holder sits outside the window, and edit_asset/edit_assessment write to that person. The probability is low — it needs a >100-hit substring and an exact-name collision across the boundary — but the outcome is precisely the silent wrong write the package comment promises never to make. Comparing Total against len(Results) and turning truncation into a self-correcting error ("more than 100 matches; narrow the name or pass the UUID") closes it.

4. includeDisabled=false is invisible to the model (§6.3, §7.5). A deactivated leaver can never resolve by name or username now, but userNotFoundHint lists only the accepted input forms and get_user_id_by_name's description doesn't mention enabled-only. The model will retry name variants forever. Worth one clause in the description and in the not-found hint. Related: the email path goes to /rest/2.0/users/email/{email}, which isn't filtered for enabled state in this code — so the same person may resolve by email but not by name. Whichever way that endpoint behaves, say so.

5. Nothing cites where the nameSearchFields contract came from (§8.2). Five enum values plus includeDisabled are hard-coded with a comment asserting they are "the endpoint's own defaults", and every test is a double that mirrors that assumption, so it cannot fail in CI. The string appears nowhere else in the repo, and neither the commit message nor the PR body names a spec or a deployment. If DGC rejects an unknown enum value, or wants a comma-separated list rather than repeated params, this turns username and email resolution — which works in production today through edit_asset — into a 400 on every call. §8.2 asks for the source and the version verified against, in the comment and the PR body. If that can't be obtained before merge, relying on the defaults the comment says these already are is the lower-risk shape.

Smaller points, take or leave

• resolveAssignees (both assessment tools) resolves USER entries as it walks the list, so a bad GROUP UUID later in the list is only reported after the earlier lookups have gone out — §6.1 asks for validation before any network call. Nothing is written either way; this is request cost and error ordering.
• resolveUsers in search_asset_keyword drops the resolving <label> "<value>": prefix its five sibling filters carry (§6.5). The value still appears in the resolver's own message.
• get_user_id_by_name echoes a UUID back unverified, so on that path it returns no fullName/username to confirm against and will happily echo an asset UUID as a userId. Right for the mid-chain resolver; for the standalone lookup tool a GET /rest/2.0/users/{id} on the UUID path would make the confirmation story work. The Output tag documents the gap honestly, so this is a design call.
• notFoundError's "Valid users available: …" label is accurate for the enumerable sets (status, domain type) but overstates a fuzzy user search, where the list is up to 15 near-miss display names. "Closest matches:" would read truer.
• A blank user reference errors with "a user is required" without naming which parameter — ownerId, userId, assignees[2].id or name (§6.3). create_assessment already wraps with assignees[%d]:; the others don't.

Not raised

search_asset_keyword's own one-line description is well short of §7.1, but it is pre-existing and outside this change's scope — the createdByFilter tag it touched is compliant. Empty Permissions on the new tool matches every other tool needing no specific DGC permission. And the three-way split in how an ambiguous user is treated across the surfaces is justified by their different contracts and documented at both sites.

[betty-pr-factory-app]

Review

Reviewed pesto/task-1b73776b (2 commits, b453e5b) against main (cfe51c9), with docs/TOOL_CONTRIBUTION_STANDARDS.md as the governing document.

This is good work. The shared pkg/tools/resolve package is the right shape for the problem, the "never pick a person for the model" invariant is enforced in code rather than in prose, and the test suite actually pins the behaviour that matters — precedence, ambiguity, truncation, and that no write happens when resolution fails. create_assessment and edit_assessment gained their first test files in the process, which is a bonus.

Two things need changing before merge, both small and localised. Everything else below is either a convention cleanup or a note confirming something was done as intended.

Verified here

go build ./...                                        pass
go vet  (resolve, get_user_id_by_name, clients, search_asset_keyword)   pass
go test resolve, get_user_id_by_name, clients         ok  0.018s / 0.012s / 0.061s
go test search_asset_keyword, edit_asset,
        create_assessment, edit_assessment, pkg/tools ok  0.062s / 0.181s / 0.032s / 0.058s / 0.296s

Not verified: no Collibra instance was reachable from the review environment, so the /rest/2.0/users contract this change leans on — the meaning of total, the nameSearchFields enum, includeDisabled, and whether GET /rest/2.0/users/email/{email} also returns deactivated accounts — was not exercised against a live deployment. The PR says so itself in the package comment (pkg/clients/edit_asset_client.go:742-759) and flags the §8.3 contract test as owed; that is the right way to handle it, and I'm noting it rather than raising it.

---

1. get_user_id_by_name's description has no "when not to use it" — and it cannot resolve a user group (§7.1 point 3, §7.3, §6.3)

pkg/tools/get_user_id_by_name/tool.go:37-48

The description is strong on six of the seven §7.1 points — what it does, ordering, key parameter, what it returns, side effects, and four example questions including a vague one. What's missing is the boundary: it never says what the tool is not for, and never names the neighbour it will be confused with.

That matters concretely, because the tool resolves users only while every tool it feeds also takes groups:

• create_assessment / edit_assessment require a GROUP assignee's UUID, and nothing in CHIP resolves a group name.
• edit_asset's userId tag says the role is assigned to "the user (or user group)".

So a model asked "make the Data Stewards group the owner" will call get_user_id_by_name("Data Stewards") and get back userNotFoundHint, which lists four user forms and says nothing about groups being out of scope or where a group UUID comes from. It will most likely retry name variants.

Second half of the same gap: the description never mentions search_asset_keyword as the way to find a person this lookup can't resolve — even though pkg/tools/resolve/user.go:106 tells the model to do exactly that on a truncated search. §7.3 asks the description to stand alone.

Please add a clause to the Description saying it resolves users only, that a user group is not resolvable by name and must be given as its UUID, and that search_asset_keyword with resourceTypeFilters: ["User"] (or ["UserGroup"]) is where to go when this lookup can't help. And add the group carve-out to resolve.userNotFoundHint (pkg/tools/resolve/user.go:26) so the error is self-correcting too, not just the tool description — resourceTypeFilters does support both values (search_asset_keyword/tool.go:17), so the advice is real.

2. The truncation guard is only half-applied in dq_notifications (§6.5, §6.3)

pkg/clients/dq_notifications.go:224-245

resolve.userByName refuses to resolve by display name whenever the page is truncated, regardless of what happens to be inside the window (pkg/tools/resolve/user.go:104-108) — correct, and the reasoning in that comment is exactly right. findRecipientByName only reaches the same conclusion when it already found a display-name match:

if match != nil && search.Truncated {
    return nil, true, nil
}

When the page is truncated and no exact display-name match happens to fall inside the window, it returns (nil, false, nil). The recipient then lands in Unresolved but not Ambiguous, so UnresolvedRecipientsMessage tells the user:

These notification recipients have no active Collibra account: Smith.

which is not something we know — the window was truncated, so nothing was learned about accounts outside it. It also sends the user hunting for a typo instead of telling them to pass a username or email. Reachable for any recipient string that partial-matches more than userSearchLimit (100) accounts, i.e. a common surname.

The fix is to hoist the check above the display-name scan, right after the exactUsernameMatch early return (which is safe to keep first — usernames are unique, so truncation can't hide a second holder):

if u := exactUsernameMatch(search.Users, name); u != nil {
    return u, false, nil
}
if search.Truncated {
    return nil, true, nil
}

…and drop the now-redundant trailing check. Worth a test mirroring TestUserRef_TruncatedSearchDoesNotResolveByName: a total=250 page whose users' FullName() doesn't equal the recipient, asserting it lands in Ambiguous. Since Ambiguous would then cover "too many matches to be sure" as well as "two people share it", the shared-name sentence in UnresolvedRecipientsMessage may want slightly broader wording.

---

Minor — happy for these to land in the same pass or not at all

Stepdown rule (AGENTS.md, cited from §-line 11 of the standards). New helpers were inserted ahead of pre-existing ones that the handler calls earlier. In create_assessment/tool.go the handler calls resolveTemplateID (85) → resolveOwnerID (92) → resolveAssignees (96) but they're defined 130 / 144 / 179; in edit_assessment/tool.go resolveAssessmentID is called first (156) and now defined at 341, behind the new resolveAssignees (309). Pure movement.

Leftover duplicate. resolve.SuggestionSuffix is now exported and the package doc justifies itself as "one small shared package rather than a copy per tool" — but pkg/tools/edit_asset/tool.go:33 still holds a byte-identical private copy with the same signature, in a package that already imports resolve. Its five call sites in operations.go could point at the shared one. (create_asset/tool.go:633 is a different two-arg variant — out of scope.)

---

The four deliberate decisions — all four done as described

No experimental flag on get_user_id_by_name. Registered outside every gate at register.go:100-108, with a comment naming §3.1 and the §1.1 half-a-pair reasoning. The premise holds: create_assessment, edit_assessment, edit_asset and search_asset_keyword are all registered unconditionally in the same run; the file's only gated block is ContextSpecificationsFeature. cmd/chip/experimental.go is untouched, which is right — §3.3 governs a flag that exists. register_test.go:31-40 asserts visibility with an empty config, and TestRegisterAll_AllToolsHaveProperAnnotations picks the new tool up automatically.

Ambiguity is an error, not a payload. ambiguousError returns a Go error naming every candidate as Name (id UUID, username: x); the handler returns Output{}, err and the test asserts out.UserID == "" alongside it. Every downstream site aborts before its write, and each is tested — TestCreateAssessment_AmbiguousOwnerCreatesNothing, TestEditAsset_SetResponsibility_AmbiguousFullNameWritesNothing (asserts nothing was created), TestCreatedByFilterAmbiguousNameReturnsCandidates, and edit_assessment's atomic phase-3 abort. dq_notifications is the one place ambiguity isn't an error, and that's correct for a surface that already returns needs_input — splitting Ambiguous out of Unresolved so the two get different advice is the right call there.

Email in, never out. Checked structurally rather than by reading intent: EmailAddress is referenced in exactly one place in non-test code, its own field declaration (edit_asset_client.go:721). Nothing reads it. resolve.User carries ID/UserName/FullName only, toUser drops the email explicitly, candidate lines carry username: and never an address, and Output has no email field — with two tests asserting the ambiguity error contains no @ at all. The only email that can surface is the caller's own input echoed back in a failure message, which is the model's own string and is what makes the error self-correcting under §6.3. Not a leak.

The pickMatch extraction is genuinely a move. Compared old and new side by side: normalize and SuggestionSuffix bodies are identical, pickMatch's two error branches are lifted verbatim into notFoundError/ambiguousError, and namedRef{id,name,ctx} → NamedRef{ID,Name,Ctx} accounts for most of the diff. The strings are unchanged for the five pre-existing filters: filterHints(param) supplies exactly the old "pass the UUID in %s to disambiguate", and with empty Candidates/NotFound hints notFoundError falls back to "Valid "+label+"s" and appends nothing. createdByFilter is the only branch actually rewritten, which is the point. Existing search_asset_keyword tests pass untouched.

One thing to be explicit about since it shows up in the moved code: the %ss pluralisation (Valid statuss, %d statuss share that name) came across with the move and is pre-existing on main. Not a defect of this PR and not something I'm asking you to fix here.

Note on the previous round

No previous-round findings file was handed to this node (only open_pr and push_branch inputs were present), so I couldn't check each earlier finding off against a list. I did review the remediation commit on its own merits: b453e5b is a real fix rather than a paper-over — it adds clients.UserSearch{Total, Truncated}, refuses to reduce by display name over a truncated page, deliberately keeps the unique-username path resolving over one, and lands two tests that fail without the change. Finding 2 above is precisely where that fix wasn't carried through to the second call site over the same search.