fleet/fleetd
3
0
Fork 0
Code Issues 97 Pull Requests 2 Actions Packages Projects Releases 1 Wiki Activity

fleetd #759: fix fleet_reply comment and hand-copied role list #764

Closed
agent wants to merge 0 commits from worker/759-authz-comment-and-role-list-e3f0e5-5 into main
pull from: worker/759-authz-comment-and-role-list-e3f0e5-5
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-05 10:45:07 +02:00
Member
Copy Link
Copy Source

Finding 4: FleetMcp.java's comment above the fleet_reply handler said the authz check asks whether the caller is a worker at all. The real check (Authz.java, case REPLY, ASK) is terminal ownership via Principal.ownsSession, not a role test. Rewrote the comment to state that.

Finding 5: fleet_list's tool description hardcoded 'architect/dev/reviewer', missing hunter. Added MemberRole.wireNames() (factored out of parse()'s error-message builder, which now reuses it) and used it in the description instead of the literal string.

Build: mvn clean install -> BUILD SUCCESS, Tests run: 2140, Failures: 0, Errors: 0, Skipped: 0. MemberRoleTest (7 tests) passed unchanged, including parseRejectsAnUnknownRoleAndListsTheValidOnes; the error message format is unchanged (same ", " separator, same order).

Finding 4: FleetMcp.java's comment above the fleet_reply handler said the authz check asks whether the caller is a worker at all. The real check (Authz.java, case REPLY, ASK) is terminal ownership via Principal.ownsSession, not a role test. Rewrote the comment to state that. Finding 5: fleet_list's tool description hardcoded 'architect/dev/reviewer', missing hunter. Added MemberRole.wireNames() (factored out of parse()'s error-message builder, which now reuses it) and used it in the description instead of the literal string. Build: mvn clean install -> BUILD SUCCESS, Tests run: 2140, Failures: 0, Errors: 0, Skipped: 0. MemberRoleTest (7 tests) passed unchanged, including parseRejectsAnUnknownRoleAndListsTheValidOnes; the error message format is unchanged (same ", " separator, same order).
agent added 1 commit 2026-10-05 10:45:07 +02:00
fleetd #759: fix the fleet_reply comment and the hand-copied role list
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 44s
Details
CI / build (pull_request) Failing after 1m59s
Details
CI / shell-tests (push) Failing after 10s
Details
CI / contract (push) Successful in 55s
Details
CI / build (push) Failing after 1m59s
Details
2289e94223 
Finding 4: the comment above fleet_reply's handler claimed the authz check
asks whether the caller is a worker at all. It actually checks terminal
ownership (Authz.java REPLY/ASK -> caller.ownsSession), which is why an
observer can reply on its own pane with no role test involved.

Finding 5: fleet_list's tool description hardcoded 'architect/dev/reviewer',
missing hunter. Added MemberRole.wireNames() (pulled out of parse()'s error
message builder, which now calls it too) and used it in the description so
the list can't drift again.
ltms commented 2026-10-05 10:51:19 +02:00
Owner
Copy Link
Copy Source

Merged locally to main at 2289e94 and pushed. Closing this PR rather than merging it in the forge, as this project does.

What I verified myself

  • Tree parity. Merged into a throwaway worktree off main — a fast-forward. git rev-parse 'HEAD^{tree}' on the merge and on the worker branch both give 9c98c075, so the bytes I tested are the bytes that shipped.
  • Build. cd fleetd && mvn clean install, exit code captured with MVN_EXIT=$? and not piped: MVN_EXIT=0, BUILD SUCCESS. I summed the surefire XML rather than trusting a console line: 178 files, 2140 tests, 0 failures, 0 errors, 0 skipped. main was also 2140, which is right — this change adds no test.
  • The report matched the diff, line for line. I read the whole diff before building.

Finding 4 is correct

The old comment claimed the check was "is this caller a worker at all". Authz.java:162 is case REPLY, ASK -> caller.ownsSession(targetSession) and Principal.ownsSession is terminal != null && terminal.equals(sessionId). No role is consulted, which is exactly why an observer can reply for its own pane. The replacement states that in three lines, names no other class and no test class.

Finding 5 is correct, and better than I asked for

parse's inline StringBuilder loop is gone and parse now calls the new MemberRole.wireNames(), so there is one builder. MemberRoleTest.parseRejectsAnUnknownRoleAndListsTheValidOnes asserts the message contains architect, dev, hunter, reviewer and passes unchanged — so that test now transitively proves wireNames() returns exactly that string, and therefore that the fleet_list description includes hunter. That is a proof through a passing test, not an inference from reading.

The naming also happens to match a convention already in the codebase: FleetTool.wireNames() plays the same role for the tool-name list, and McpContractDocTest reads it. I did not ask for that and did not know it; good consistency.

A gap I found that neither of us had a test for

I mutated the fix: I put the stale "architect/dev/reviewer" literal back into the description and ran FleetMcp*Test,MemberRoleTest — 185 tests, 0 failures, BUILD SUCCESS. The mutation survives.

So the MemberRole half is pinned and the FleetMcp half is not. Someone can revert the description to a hand-copied literal and no test notices — the same hole that let hunter go missing in the first place. The fix is still a clear improvement, because the copy is now derived rather than typed, but "one fact, one place" is holding by author discipline here, not by a gate.

I am not forcing a test in with this merge. listTool() is private static, so pinning it means either widening it for a test — which is what code-quality rule 4 exists to stop — or standing the server up in a test. That is a design call, not a tidy-up, and it belongs in its own ticket with the other build-gate work. Recorded on #759 so it is not lost.

I restored the mutated file and re-checked the tree hash before pushing: still 9c98c075, working tree clean.

Thank you for the honest report — naming the exact test that covers the refactor, and saying plainly that you had not run the sync check and why, is what let me verify this quickly instead of re-deriving it.

Merged locally to `main` at `2289e94` and pushed. Closing this PR rather than merging it in the forge, as this project does. ## What I verified myself - **Tree parity.** Merged into a throwaway worktree off `main` — a fast-forward. `git rev-parse 'HEAD^{tree}'` on the merge and on the worker branch both give `9c98c075`, so the bytes I tested are the bytes that shipped. - **Build.** `cd fleetd && mvn clean install`, exit code captured with `MVN_EXIT=$?` and not piped: `MVN_EXIT=0`, `BUILD SUCCESS`. I summed the surefire XML rather than trusting a console line: **178 files, 2140 tests, 0 failures, 0 errors, 0 skipped**. `main` was also 2140, which is right — this change adds no test. - **The report matched the diff**, line for line. I read the whole diff before building. ## Finding 4 is correct The old comment claimed the check was "is this caller a worker at all". `Authz.java:162` is `case REPLY, ASK -> caller.ownsSession(targetSession)` and `Principal.ownsSession` is `terminal != null && terminal.equals(sessionId)`. No role is consulted, which is exactly why an observer can reply for its own pane. The replacement states that in three lines, names no other class and no test class. ## Finding 5 is correct, and better than I asked for `parse`'s inline `StringBuilder` loop is gone and `parse` now calls the new `MemberRole.wireNames()`, so there is one builder. `MemberRoleTest.parseRejectsAnUnknownRoleAndListsTheValidOnes` asserts the message contains `architect, dev, hunter, reviewer` and passes unchanged — so that test now transitively proves `wireNames()` returns exactly that string, and therefore that the `fleet_list` description includes `hunter`. That is a proof through a passing test, not an inference from reading. The naming also happens to match a convention already in the codebase: `FleetTool.wireNames()` plays the same role for the tool-name list, and `McpContractDocTest` reads it. I did not ask for that and did not know it; good consistency. ## A gap I found that neither of us had a test for I mutated the fix: I put the stale `"architect/dev/reviewer"` literal back into the description and ran `FleetMcp*Test,MemberRoleTest` — **185 tests, 0 failures, BUILD SUCCESS**. The mutation survives. So the `MemberRole` half is pinned and the `FleetMcp` half is not. Someone can revert the description to a hand-copied literal and no test notices — the same hole that let `hunter` go missing in the first place. The fix is still a clear improvement, because the copy is now derived rather than typed, but "one fact, one place" is holding by author discipline here, not by a gate. I am **not** forcing a test in with this merge. `listTool()` is `private static`, so pinning it means either widening it for a test — which is what code-quality rule 4 exists to stop — or standing the server up in a test. That is a design call, not a tidy-up, and it belongs in its own ticket with the other build-gate work. Recorded on #759 so it is not lost. I restored the mutated file and re-checked the tree hash before pushing: still `9c98c075`, working tree clean. Thank you for the honest report — naming the exact test that covers the refactor, and saying plainly that you had not run the sync check and why, is what let me verify this quickly instead of re-deriving it.
ltms closed this pull request 2026-10-05 10:51:27 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 44s
Details
CI / build (pull_request) Failing after 1m59s
Details
CI / shell-tests (push) Failing after 10s
Details
CI / contract (push) Successful in 55s
Details
CI / build (push) Failing after 1m59s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#764
Delete Branch "worker/759-authz-comment-and-role-list-e3f0e5-5"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?