Conversation
Self-review of #254. The team area injected ProjectRoleService and AdminProjectService directly. Every one of the 68 cross-module imports in onboarding goes through `user.external`; these were the first that did not, and they reached past the boundary that package exists to be. Nothing failed, because there is no Modulith verification test to fail. That is the reason to fix it rather than a reason not to: the convention is currently absolute and holds only as long as nobody is the first to break it. `ProjectRoleApi` now publishes the whole catalogue, the roles somebody holds on a project, and the two project-scoped role writes. `ProjectMembershipApi` gains the two membership writes, delegating to the service that owns their rules rather than restating them. Both published surfaces are deliberate subsets: creating and deleting roles, editing role skills, and the projectless role overloads are all absent, so a caller outside the module cannot reach them even by accident -- which is a stronger guarantee than the test that previously asserted we did not call them, and the test now says so. The role operations moved into a ProjectRoleApiService adapter rather than onto ProjectRoleService, matching how ProjectMembershipApiService is already split out, and keeping the service within its function budget. Also from the read-back: assign_project_role compared the role by name where it had the id in hand. Names are unique in the schema so it was not wrong, but it leaned on a constraint in another module without saying so; it compares ids now. And the "role(s)" in the unassign preview is a sentence a person would write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-reviewRead back against Fixed: the team area reached past the module boundary
I only noticed how sharp this was after counting: every one of the 68 cross-module imports in In fairness to the issue: #226 names those services by path, and following it literally is how this got in. The spec was pointing at where the behaviour lives, not prescribing the import. What is published now:
Both are deliberate subsets. Creating and deleting roles, editing role skills, and the projectless role overloads are all absent, so a caller outside the module cannot reach them even by accident. That is a stronger guarantee than the test I had written, which asserted only that we did not call the projectless form — the test now asserts the published surface has no such form to call. The role operations went into a new Two smaller things from the same read
What I checked and left alone
Checks
For the reviewerThe published-API change touches |
Adds the structure half of the content area (backend#228): get_member_path, add/update/delete for phases, steps, tasks and resources, and reset_member_path. 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 lets it through only when the owner is on the turn's project, at proposal and again at confirm. An element that is missing and one on somebody else's path get the same refusal, so a refusal cannot be used to probe for ids. Team mode never sets a hire's progress. update_task carries `finished` through as the task has it at confirm time, there is no tool that starts, finishes or ticks anything, and the two places where a write does touch progress say so in the preview: adding a task to a finished step reopens it, and deleting a phase, step or path names the finished work that goes with it. Updates store only the fields that change and apply them over the element as it is at confirm, so an edit made since the preview is not undone. Resource URLs must be http(s), since a hire clicks them. A member on several projects has one path; every preview says so. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd packets Adds the rest of the content area (backend#228): list_pending_skips, list_feedback, get_phase_checks and get_orientation_packet, and the actions accept_skip, deny_skip, delete_skip, mark_feedback_read, replace_phase_checks, author_orientation_packet and revert_orientation_packet. Accepting and denying a skip are the two actions in this area that change a hire's progress, as the admin surface they mirror does, so their previews say what happens to the step: an accepted skip marks it skipped and can finish the hire's onboarding, a denied one puts a step they had started back to waiting. A denial without a comment is refused because the hire is shown it, and the comment is quoted in full in the preview. replace_phase_checks takes the whole list because the service deletes any question it is not given, along with everybody's answers to it. Every id is checked against what the phase has, the preview lists what is kept, new and deleted, and a confirm is turned down if the phase's questions changed since. Orientation packets are keyed by task and project but the service only checks that the task exists. ContentScope resolves the task through its repository's project links, at proposal and at confirm. Authoring shows the full text the hire will read and what it replaces, and a person's packet is named as such; a confirm is turned down if the packet was replaced since the preview. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The team area: three reads (list_project_roles, get_member_roles, find_user_to_add) and four actions the manager confirms by id -- add_members, remove_member (destructive), assign_project_role and unassign_project_role. `assignUsers` and `removeUser` sit behind admin-only REST routes, so a manager cannot reach them at all except here, where the turn has already proved they manage this project. Every target is therefore resolved against this project's membership at draft and again at confirm, and the project is always the turn's. Roles use the three-argument service methods only; the project-less overloads apply a role across every project somebody belongs to and are never called. Removing somebody takes their roles with them, and the preview says so. The roles hang off the membership row, so deleting it deletes them, and adding the person back later gives them a fresh membership with none -- a manager confirming "remove from project" would otherwise be confirming something they were not told about. Two guards the services do not provide. `removeUser` refuses the project's manager with a 409 at write time; that is refused at draft instead, so it is never offered, which needs knowing who the manager is rather than whether somebody may manage -- those differ for every administrator, hence ProjectMembershipApi.getProjectManagerId. `unassignRoleFromUser` removes by id without complaining when nothing matches, so a role somebody does not hold would confirm as a change and change nothing; the draft checks what they actually hold. find_user_to_add is the one read here that is not about this project, because adding somebody means naming a person the manager cannot yet see. It is held to exactly that by three rules: exact match on email or GitHub login, at most one result, and two fields back. A partial identifier matches nobody, so it cannot be walked, and two matches are answered as nobody rather than as a guess about who was meant. Expressed as a Specification on equality rather than a finder, and proved against a real database rather than a mock, since the predicate is the part that would leak. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… deleted Self-review of the content area found four places where a preview said something other than what happens, or a write would not have happened at all. - reset_member_path would have failed on every confirm. OnboardingPath's deleteByUserId is a derived delete, which throws TransactionRequiredException when it runs without a transaction, and neither the service nor the buddy's confirm path has one. It is now @transactional on the repository, with a test that runs outside the test's own transaction, which is what hid it. The admin endpoints DELETE /users/{userId}/path and DELETE /me/path reach the same method and had the same problem. - replace_phase_checks said a removed question is deleted "with everybody's past answers". Attempts reference a question by plain id with no foreign key, so the answers stay as rows and simply stop counting. It now says that. - Deleting the last phase, step or task said the ones after it move up. It now says so only when there are later siblings. - get_member_path listed phases the hire is never shown, as if they were live. They are marked as not shown. Adds a mount test for the acceptance criteria that are about the set of tools: exactly these tools, no two sharing a name (handlers are keyed by it), and none that can tick a task or start, finish or complete a step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Self-review of #254. The team area injected ProjectRoleService and AdminProjectService directly. Every one of the 68 cross-module imports in onboarding goes through `user.external`; these were the first that did not, and they reached past the boundary that package exists to be. Nothing failed, because there is no Modulith verification test to fail. That is the reason to fix it rather than a reason not to: the convention is currently absolute and holds only as long as nobody is the first to break it. `ProjectRoleApi` now publishes the whole catalogue, the roles somebody holds on a project, and the two project-scoped role writes. `ProjectMembershipApi` gains the two membership writes, delegating to the service that owns their rules rather than restating them. Both published surfaces are deliberate subsets: creating and deleting roles, editing role skills, and the projectless role overloads are all absent, so a caller outside the module cannot reach them even by accident -- which is a stronger guarantee than the test that previously asserted we did not call them, and the test now says so. The role operations moved into a ProjectRoleApiService adapter rather than onto ProjectRoleService, matching how ProjectMembershipApiService is already split out, and keeping the service within its function budget. Also from the read-back: assign_project_role compared the role by name where it had the id in hand. Names are unique in the schema so it was not wrong, but it leaned on a constraint in another module without saying so; it compares ids now. And the "role(s)" in the unassign preview is a sentence a person would write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dev changed OnboardingPathService.getOnboardingPathByUserId to return the path as the person has it. Phases whose generation produced nothing are no longer listed; they come back as generationIssues instead. get_member_path now lists those under "not shown to them" with their ids, in place of the per-phase marker that could no longer fire. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
734dfae to
20f2992
Compare
BrainSkript
left a comment
BrainSkript
left a comment
There was a problem hiding this comment.
Review: PR #254 — "Let the team-mode buddy manage who is on a project and what they do"
PR: #254
Closes: #226 (Team-mode actions: members and project roles)
Author: Linus Bauer
Branch: feature/226-team-actions → feature/227-arrival-actions (stacked on #253; retarget to dev after #253 merges)
Size: +1488 / −7 across 12 files (9 production, 3 test)
CI: green (build ×2, gitleaks ×2)
1. Summary
The PR mounts team mode's TEAM area: three read tools (TeamMemberTools) and four action
handlers (TeamMemberActions), plus the user-module surface they need — new methods on
ProjectMembershipApi, ProjectRoleApi and UserApi, a new ProjectRoleApiService that takes
over the published interface from ProjectRoleService, and an exact-match directory lookup on
UserApiService.
Every acceptance criterion of ticket #226 is met and pinned by a test:
open_area(TEAM)mounts exactly the ticket's three reads and four actions.find_user_to_addis exact-or-nothing; partials, wildcards and blank input find nobody — proven
against a real database (@DataJpaTeston H2), which is the right choice since the predicate,
not the mapping, is the security boundary.- Nothing mutates while drafting (
verify(exactly = 0)tests on every write path). - Removing the project's manager is refused by name at draft and again at
recheck. - Assigning a role to a non-member is refused before the service is called.
- Only project-scoped role operations are reachable; a reflection test pins that every
*RoleOnProjectmethod on the published API takes a project. Catalog operations are not mounted. - Confirm re-checks manager (
BuddyProposalService.attempt→canManageProject) and membership
(recheck).
One deliberate deviation from the ticket's letter, and it is the right one: the ticket's "Calls"
column names concrete user-module services (ProjectRoleService, AdminProjectService), but the
PR reaches them through the published user.external APIs instead. Calling concrete services
across modules would have violated the modulith boundary rules in AGENTS.md.
The security reasoning in the PR body checks out against the code: AdminProjectService.removeUser
does 409 on the project's manager and 404 on a non-assignment; assignUsers is idempotent and
404s on missing users; BuddyTeamService.authorize confirms canManageProject before a turn and
BuddyProposalService re-confirms it at claim time. The distinction between
getProjectManagerId (who is the manager) and canManageProject (who may manage — true for
every admin) is real and correctly motivated.
Verification run locally on the PR head (20f2992): the 47 new tests — 28 actions
(TeamMemberActionsTest), 12 tools (TeamMemberToolsTest), 7 directory lookup
(UserDirectoryLookupTest) — all pass, 0 failures, matching the PR's claim exactly. detekt and
ktlintCheck clean. The full ./gradlew check (3324 tests) was not re-run; CI covers it.
Verdict: approve with minor comments. No blocking issues.
2. Correctness issues
C1 — UnassignProjectRoleAction.recheck doesn't re-check that the role is still held (minor)
draft refuses a role-not-held precisely because the service "removes by id without complaining
when nothing matches", so it "would confirm as a change and change nothing". But recheck only
re-verifies membership. If the role is unassigned elsewhere between preview and confirm, the
confirm still reports "Done. 'X' is off them on this project." for a change that had already
happened. The end-state message happens to remain true, so the impact is cosmetic — but the PR's
own stated principle argues the recheck should mirror the draft check. The same consideration
applies, more weakly, to AssignProjectRoleAction (a no-op assign still reports "They hold X now",
which is also still true).
C2 — AddMembersAction.recheck doesn't re-verify the users still exist (minor)
A user deleted between draft and confirm makes perform fail with the 404 from assignUsers,
which BuddyProposalService.runClaimed records as FAILED with the reason shown. Acceptable
degradation — just noting the asymmetry with the draft's careful "not a person any more" refusal.
3. Architecture issues
None. Module boundaries are clean throughout:
- The onboarding module touches only
user.externalinterfaces; no cross-module repository or
concrete-service access. - Entities are mapped to boundary DTOs (
ProjectRoleDetailDto,DirectoryMatch) before crossing
the module line;DirectoryMatchcarrying exactly two fields instead of aUserDtois a
well-reasoned minimization, pinned by a test. - The
ProjectRoleApiServicesplit mirrors the existingProjectMembershipApiServicepattern,
and after the refactorProjectRoleServiceis only injected directly inside its own module
(ProjectRoleController). - The write delegations (
ProjectMembershipApiService→AdminProjectService,
ProjectRoleApiService→ProjectRoleService) cross bean boundaries, so the callees'
@Transactionalsemantics apply correctly. - No new dependencies, no build-file changes.
4. Test gaps
- The new API-service methods have no direct tests:
ProjectMembershipApiService.getProjectManagerId(project missing → empty; project with no
manager → empty), theaddMembers/removeMemberdelegation, andProjectRoleApiService's
entity→DTO mappings (getAllProjectRoles,getRolesOnProject). They are thin and indirectly
covered through the mocked-API action tests, butgetProjectManagerIdcontains real
null-handling logic that nothing currently pins. - Everything else is well covered, and the
@DataJpaTestfor the lookup contract is exactly the
right test choice — a mock would have asserted the mapping and said nothing about the predicate.
5. Documentation gaps
- Stale KDoc on
ProjectRoleService.getProjectRolesByIds(the one finding to fix before
merge): it still says "Implementation of the module-facing [ProjectRoleApi]", but the class no
longer implements that interface — the override moved toProjectRoleApiService. The sentence
should be dropped or reworded. - Trivial:
ProjectMembershipApi.removeMember's@throwslists 404 for "not on the project", but
the underlyingremoveUseralso 404s when the project itself doesn't exist. - No ADR needed — the PR follows existing patterns rather than introducing an architectural
decision. (Note for the review process: the repo has nodocs/adrs/directory and no ticket or
plan file for #226 underdocs/, so this review compared against GitHub issue #226 directly.)
6. API issues
No REST surface changes, so no OpenAPI concerns. One question on the ticket's wording: the ticket
asks that the remove preview "names the member and the project" — the preview says "this
project" without the project's name. Since a team-mode session is scoped to one project, that is
unambiguous in context; just confirming it is a conscious reading of the requirement.
7. Suggested fixes
- Fix the stale KDoc on
ProjectRoleService.getProjectRolesByIds(drop or reword the
"Implementation of the module-facing ProjectRoleApi" sentence). - Consider adding the role-still-held check to
UnassignProjectRoleAction.recheckfor symmetry
with the draft guard (C1). - Consider a small unit test for
ProjectMembershipApiService.getProjectManagerId's empty cases. - Optional: the two-line
memberOnhelper is duplicated privately inTeamMemberActionsand
TeamMemberTools— could be one shared internal helper. AndAddMembersAction.draftcould
batchgetUsersByIds(ids)instead of one call per id (bounded and draft-only, so purely
cosmetic).
Review of #254, findings C1, C2, the stale KDoc and the two cleanups. `unassign_project_role` refuses a role somebody does not hold, because the service removes by id without complaining and the confirm would report a change that did not happen. The recheck did not ask again, so a role taken off between preview and confirm still reported as this proposal's doing. It asks now, and says the role is already off them. `assign_project_role` gets the mirror of it: assigning twice is not an error either, so a role given elsewhere in the meantime is no longer reported as this confirm's work. `add_members` resolved each person to refuse an account that is not a person any more, then confirmed without looking again -- a deletion in between reached the manager as the service's 404 rather than as a sentence. The recheck resolves them too. The draft now reads the whole batch in one directory call instead of one per id. `ProjectRoleService.getProjectRolesByIds` still described itself as the implementation of the module-facing API it no longer implements, and `ProjectMembershipApi.removeMember` documented only one of the two things its 404 means. The membership lookup and the roles-on-project read were written out three times between the tools and the actions; both are one helper now. `ProjectMembershipApiService.getProjectManagerId` answers the same empty for a project that does not exist and for one nobody manages, which nothing pinned -- a caller refusing to remove "the manager" must not read that empty as a match. Tests for both API services cover it, along with what crosses the module line and in what shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All of it addressed in a7374a2. Suggested fix 1 — stale KDoc on C1 — C2 — Suggested fix 3 — Suggested fix 4 — the two cleanups. Both done. §6 — the remove preview saying "this project" rather than the project's name. That one is deliberate, and I left it. A team-mode turn is scoped to one project the manager selected, and every other preview in every area uses the same "this project" wording; naming it in one preview would read as though the others might mean a different one. It would also mean a project lookup purely to restate something the manager is already looking at. Happy to change it if you would rather the ticket's wording be taken literally. |
Keep ProjectRoleService free of the ProjectRoleApi implementation (it lives in ProjectRoleApiService) while taking dev's industry and skill-suggestion dependencies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tructure-actions Let the team-mode buddy manage the onboarding content of a project's members
2 participants
Related issue
Closes #226
Short summary
Adds the team area of the team-mode buddy: a manager can ask who is on the project and what they do here, and change both. Every change is a stored proposal the manager confirms by id.
list_project_roles,get_member_roles(member_id),find_user_to_add(email_or_github_login).member_idcomes from thefind_memberread that already exists, so this adds no way to enumerate the project's people.add_members(standard),remove_member(destructive),assign_project_role,unassign_project_role(standard).assignUsersandremoveUsersit behind admin-only REST routes. They are reached only because the turn already proved the caller manages the project, and every target is resolved against this project's membership at draft and again at confirm.ProjectMembershipApiandProjectRoleApigain what is needed (includinggetProjectManagerId), with aProjectRoleApiServiceadapter.onboardingimports nothing fromuser.service.assignRoleToUser/unassignRoleFromUserare not called, and a test asserts it.Checks
Additional notes
devnow that Let the team-mode buddy author a project's arrival list #253 is merged.removeUseronly refuses the project's manager at write time (409), so the draft refuses it instead, using who the manager is (getProjectManagerId), not whether somebody may manage, which is true for every admin.unassignRoleFromUsersilently ignores a role somebody does not hold, so the draft checks what they actually hold.add_membersre-resolves the people as well, so an account deleted in between is a sentence rather than the service's 404.find_user_to_addis the one read that leaves the project. It matches exactly (email or GitHub login, case-insensitive), returns at most one person, and returns only a user id and a display name. It is aSpecificationoncb.equaland is tested against a real database, because a mocked repository would not test the predicate that would actually leak../gradlew checkgreen, detekt and ktlint clean. 63 new, including the stale-confirm cases above and the two API services' own behaviour —getProjectManagerIdanswers the same empty for a project that does not exist and for one nobody manages, which a caller refusing to remove "the manager" must not read as a match. No new controller here, so no new WebMvcTest.🤖 Generated with Claude Code