Let the team-mode buddy manage the onboarding content of a project's members

[LinseCed]

Related issue

Closes #228

Short summary

Adds the content area of the team-mode buddy: a manager can read and edit the onboarding paths of the project's members, answer their skip requests, read their feedback, and manage knowledge checks and orientation packets. Every write is a stored proposal the manager confirms.

• Five reads: get_member_path, list_pending_skips, list_feedback, get_phase_checks, get_orientation_packet.
• Twenty actions: add / update / delete for phases, steps, tasks and resources; replace_phase_checks; accept_skip, deny_skip, delete_skip; mark_feedback_read; reset_member_path; author_orientation_packet, revert_orientation_packet.
• A path belongs to a person, not a project, and the by-id services load an element without asking whose it is. PathElements walks each kind up to its owner and ContentScope only lets it through when the owner is a member of the turn's project, at proposal and again at confirm. A missing element and one on somebody else's path get the same refusal.
• Nothing can tick a task or start, finish or complete a step. Only accept_skip / deny_skip change a step's status, as the admin surface does, and their previews say what happens.
• Updates store only the fields that changed and apply them over the element as it is at confirm, so a later edit is not undone by the confirm.
• Fixes OnboardingPathRepository.deleteByUserId, which threw TransactionRequiredException outside a transaction (see notes).

Checks

• I verified the code makes sense intuitively
• The PR changes affect only this issue, no unrelated/unwanted code changes to other modules/code segments
• CI runs (./gradlew clean build, keycloack)
• New business logic is unit tested, including WebMvcTest for api controllers
• The new functionality is tested manually

Additional notes

• Stacked on Let the team-mode buddy manage who is on a project and what they do #254 (base feature/226-team-actions). Once Let the team-mode buddy manage who is on a project and what they do #254 is on dev, change the base to dev; the diff then shows only this PR's four commits (path structure; skips, feedback, checks and packets; self-review fixes; adapting to dev's new path shape).
• The "affects only this issue" box is unchecked on purpose. One change is outside this issue's tools: deleteByUserId is now @Transactional. reset_member_path would have failed on every confirm without it, and DELETE /users/{userId}/path and DELETE /me/path reach the same method and very probably had the same problem. A regression test that runs outside the test's own transaction failed before the fix.
• Two previews were wrong until self-review: removed check questions are not deleted with the hires' answers (attempts reference the question by plain id, so they stay as rows and stop counting), and deleting the last element claimed later ones move up.
• replace_phase_checks takes the whole list because the service deletes any question it is not given. Every id is checked against the phase, the preview lists kept / new / deleted, and a confirm is turned down if the phase's questions changed since. get_phase_checks is an extra read not in the issue.
• Choices to check: tools take place counting from 1; add_phase refuses a member with no path; resource and citation URLs must be http(s); deny_skip requires a comment because the hire is shown it.
• Tests: ./gradlew clean check was green locally with 3531 tests (0 failures), and CI is green. No new controller, so no new WebMvcTest. BuddyControllerTeamModeTest failed once with a ConcurrentModificationException in one local run and passed 3 out of 3 in isolation; this PR does not touch that controller and I have not found the cause.
• The test JVM is near its default heap. A new @DataJpaTest context with a different @Import caused 93 "Java heap space" failures in unrelated tests; the new repository tests use the existing slice configuration so the context is reused.
• Not tested manually.

🤖 Generated with Claude Code

[BrainSkript]

Review: PR #255 — "Let the team-mode buddy manage the onboarding content of a project's members"

PR: #255
Ticket: #228
Stacked on: PR #254 (base feature/226-team-actions; retarget to dev after #254 merges)
Head reviewed: b170fc0a

Reviewed against: GitHub issue #228 (ticket), PR #254 (stacked base — provides TeamActionHandler, BuddyProposalService, TeamToolArguments), root AGENTS.md, review mode rules. No local ticket/plan doc exists for #228; docs/tickets only covers the blueprint rework. No ADR is referenced or needed.

Verification performed: checked out the PR head (b170fc0a) in a worktree and ran the onboarding service/repository tests — 1053 tests, 0 failures, 0 errors. Also verified the base contracts (TeamActionHandler, confirm → recheck → perform), the text()/uuid() argument helpers (contentOrNull, so JsonNull args are safe), and that UpdateOnboardingStepRequest / GetOnboardingStepResponse.expectedOutcomes map a singular entity field — so the firstOrNull().orEmpty() handling in UpdateStepAction loses nothing.

---

1. Summary

The PR implements the ticket faithfully and, in places, more carefully than the ticket asked. The scope model (paths belong to people; PathElements walks every element kind to its owner; ContentScope checks membership at proposal and again at confirm) matches the ticket's core rule exactly, and the refusal for a missing element is deliberately identical to the refusal for an out-of-scope one. All seven acceptance criteria are met and tested: exact tool mount set, refusal at proposal and confirm, multi-project preview note, no progress-setting tools, mandatory deny comment, repository-link check for packet tools, and ownership-walk tests against a real database (not mocked repositories — the right call for a lazy-association walk).

Deviations from the ticket text are disclosed and reasonable: per-member skip/feedback reads instead of filtering the global lists, the added get_phase_checks read, and the out-of-scope @Transactional fix on deleteByUserId (justified — it fixes DELETE /users/{userId}/path and DELETE /me/path too, and the regression test runs outside a transaction, which is the only way this bug can reproduce).

The code follows the project's Kotlin style, keeps all tool-facing I/O as strings (no entity leakage), uses the user module only through its external APIs, and the KDoc is genuinely explanatory rather than code-repeating.

Verdict: approve with minor comments. A comment review is appropriate; requesting changes is not warranted. Nothing here blocks merge; the items below are small.

2. Correctness issues

2.1 (minor) ContentTeamTools.memberPath treats any ResponseStatusException as "no path"

runCatching { ... }.getOrElse { if (it !is ResponseStatusException) throw it; return "...has no onboarding path yet." } — if the service ever throws a ResponseStatusException for a different reason (a 500 wrapped downstream, a future 403), the model is told the member has no path and will act on that. Suggest checking it.statusCode == HttpStatus.NOT_FOUND and rethrowing otherwise. Same pattern, lower risk, in lookUpPendingSkip (getSkipById only throws 404 today, so this one is fine as-is).

2.2 (minor) ReplacePhaseChecksAction.recheck only detects id-set changes

base_ids is compared as a set of question ids, so an in-place edit of a question's text or options by someone else between preview and confirm keeps the same ids, passes the recheck, and is silently overwritten by the stored list. The preview's claim "turned down if the phase's questions changed since" only holds for add/delete. Either accept this consciously (it is arguably the right trade-off — the manager's whole-list replace wins) or include a cheap content fingerprint in base_ids.

2.3 (nit) AuthorOrientationPacketAction.perform discards readSections's error

It relies on requireNotNull(sections). Unreachable in practice because the stored params were validated at draft, but ReplacePhaseChecksAction.perform maps the same situation to a 400 ResponseStatusException — matching that would keep the two "re-read stored params" paths consistent and give a handled failure instead of an NPE if a stored proposal from an older deployment is ever malformed.

3. Architecture issues

None. Module boundaries are respected: cross-module access goes through ProjectMembershipApi / UserApi (external APIs of the user module), all repository access stays inside the onboarding module, business logic sits in the action handlers rather than any controller, and no JPA entity crosses a boundary (tool output is plain text). The ContentScope / PathElements split is a clean single place for the ownership rule.

One thing to be aware of, not a defect: ContentScope.member() calls getProjectMembers(projectId) on every element resolution, so a chat turn with several tool calls re-fetches the member list several times. If the user module's implementation is a database query per call, this is worth a look later; not worth blocking on.

4. Test gaps

4.1 (the notable one) ContentAreaMountTest verifies intent, not wiring

The test hand-builds the list of 20 handlers and feeds it to a mocked ObjectProvider, so it asserts that this list mounts correctly — but a future @Component handler with area = CONTENT that someone forgets to add to the test would be mounted in production while the test stays green. A @SpringBootTest (or a bean-names assertion) that compares the real TeamActionHandler beans for TeamArea.CONTENT against this set would make the "mounts exactly these tools" acceptance criterion self-maintaining.

4.2 Unresolved flaky test

The PR body mentions a one-off ConcurrentModificationException in BuddyControllerTeamModeTest that the author could not reproduce. Since this PR (and #254) introduces concurrent tool execution around shared collections, a CME in that area deserves a tracking issue rather than being left as a PR note.

4.3 Endpoint-level coverage for the deleteByUserId fix

OnboardingPathDeleteTest covers the repository method outside a transaction, which is the actual regression — good. There is no test proving DELETE /users/{userId}/path / DELETE /me/path work end-to-end now; the repo-level test is arguably sufficient, but the PR claims those endpoints "very probably had the same problem," so a controller-level regression test would close the claim.

Otherwise the coverage is strong: 32 new test suites, real-DB tests for the ownership walk, stale-confirm paths tested per action, and the "no progress tools" rule is pinned by a mount-level assertion, not just by reading the code.

5. Documentation gaps

None found. KDoc on the new public classes explains intent and ownership rules; the deleteByUserId change is documented at the method; no controller changed, so no OpenAPI work is due. The tool specs themselves are the model-facing API docs and are precise — including the 1-based place convention and the "whole list" semantics of replace_phase_checks. The PR body's "Scope as built" section accurately matches the code.

6. API issues

None. The tool surface is consistent: reads print the ids the actions take; refusals point the model at the read that would give it a valid id; enum arguments are declared as enums in the JSON schema. One design choice worth endorsing explicitly: refusals are indistinguishable between "does not exist" and "not on your project" — good, since tool calls are model-driven and ids are guessable.

Process note: the PR is stacked on #254 with base feature/226-team-actions — remember to retarget the base to dev once #254 merges, as the PR body says.

7. Suggested fixes (non-blocking)

1. Narrow the ResponseStatusException catch in memberPath to 404 (2.1).
2. Decide consciously on the id-set-only staleness check in ReplacePhaseChecksAction.recheck and either document the trade-off in the KDoc or hash the questions (2.2).
3. Align AuthorOrientationPacketAction.perform's re-validation failure with the 400 used in ReplacePhaseChecksAction.perform (2.3).
4. Replace the hand-built handler list in ContentAreaMountTest with a real bean-set assertion, or add a comment stating the list must be kept in sync (4.1).
5. File a follow-up issue for the BuddyControllerTeamModeTest CME flake (4.2).