fix(tui): let a resolved fleet member slot win over the legacy role label

[gaord]

Summary

Fix effective_fleet_role_with_source: it only consulted the resolved agent_profile when worker.agent_profile was empty, so a task whose role label is "manager" but whose agent_profile selects member:reviewer fell through to the legacy worker.role label and ran with the wrong (write-capable) authority. Prefer the resolved member slot; keep the legacy label only as a fallback.

Testing

• cargo fmt --all -- --check
• cargo test -p codewhale-tui --lib agent_profile_member_slot_overrides_legacy_role_label

Note: main currently has 5 pre-existing nonminimal_bool clippy errors, unrelated to this change.

---

No-Issue: fleet role precedence fix from a community report; no tracking issue

Maintainer follow-up

Recovered onto current main and finished per the review (option 1, fail closed at freeze). Commit e81b6837 on this branch, on top of @gaord's original commit (kept verbatim, author preserved).

• freeze_fleet_task_members now bail!s when an explicit worker.agent_profile selector is present and worker.role names a different posture than the selected member's role, alongside the existing unknown/ambiguous-selector bails. The message names both postures. Comparison uses canonical_public_role_name, so casing and legacy aliases of the member's own role are not conflicts.
• The member-first precedence in effective_fleet_role_with_source is kept; its comment now states that the conflict is settled at freeze and that a role_source of "task.role" means no roster member resolved at all.
• Tests: legacy_role_label_never_widens_into_a_member_write_slot (the mirror case: implement member + reviewer label errors at freeze and persists nothing), conflicting_member_and_role_label_is_rejected_at_freeze_naming_both_postures, and role_label_alias_of_the_selected_member_role_is_not_a_conflict. The author's agent_profile_member_slot_overrides_legacy_role_label is unchanged and still passes.
• CHANGELOG entry under Unreleased / Fixed (root and crates/tui/CHANGELOG.md synced).

Gate, as run:

• cargo fmt -p codewhale-tui -- --check: clean
• cargo clippy -p codewhale-tui --lib --all-targets -- -D warnings: clean
• cargo test -p codewhale-tui --lib -- fleet::worker_runtime fleet::: 366 passed, 0 failed
• cargo test -p codewhale-tui --lib: 11854 passed, 0 failed, 13 ignored

[devin-ai-integration]

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[Hmbown]

Reviewed at e6868d1df. The bug you describe is real and I reproduced it. I am requesting changes only because the fix flips the precedence unconditionally, and the mirror case silently widens authority in the other direction — which is the same harm your own test comment names.

What I verified

Detached worktree, RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib -- fleet::worker_runtime:

• On the PR head: 70 passed; 0 failed.
• Your test against the pre-fix effective_fleet_role_with_source (I spliced agent_profile_member_slot_overrides_legacy_role_label onto the 505fc7a27^ version of the file): FAILS, left: Some("manager"), right: Some("reviewer"). So member:reviewer really did fall through to the "manager" label. Confirmed.

I also traced why it is reachable: freeze_fleet_task_members (worker_runtime.rs:217-221) deliberately keeps a divergent worker.role when an explicit worker.agent_profile selector is present — it only canonicalizes the string:

worker.agent_profile = Some(format!("member:{}", profile.id));
if explicit_selector.is_none() {
    worker.role = Some(snapshot.role.clone());
} else if let Some(role) = worker.role.as_mut() {
    *role = canonical_public_role_name(role.trim());   // divergence preserved
}

So a task can carry agent_profile: member:X and role: Y past freezing with X.role != Y, and nothing rejects it. Someone has to lose. Today the label wins; after this PR the member wins.

The concern: the mirror case widens write authority

I added a throwaway probe test (member alice whose slot is implement; task worker.role = "reviewer") and ran it against both trees. Same input, fleet_task_to_worker_spec_with_profiles:

	spec.role	spec.agent_type	runtime_profile.permissions.write
origin/main	reviewer	Reviewer	false
this PR	implement	Builder	true

That is a read-only task quietly becoming write-capable. It is the exact failure your test guards against —

"reviewer authority must not be silently widened to a write-capable worker"

— just approached from the other side. Net, the change trades one silent widening for another rather than closing the class.

(For completeness: manager is not a canonical FleetRole, so it falls to FleetRole::Worker via fleet_role_to_agent_type — write-capable. Your case is genuinely a widening and genuinely worth fixing. I am not disputing the direction, only the unconditional rule.)

What I would like instead

Any one of these closes the class rather than rotating it:

1. Fail closed at freeze time (my preference). In freeze_fleet_task_members, when explicit_selector.is_some() and worker.role names a different posture than profile.role.name, bail! the way the surrounding code already bails for unknown selectors and ambiguous members (worker_runtime.rs:168-199). A conflicting spec is an authoring error; resolving it silently either way is how you get a worker running with authority nobody wrote down. This also keeps effective_fleet_role_with_source honest — by the time it runs, there is no conflict left to arbitrate.
2. Narrower wins. If you want conflicts to stay legal, resolve to the less authoritative posture rather than to a fixed side. That satisfies your case (reviewer < manager) and mine (reviewer < implement) with one rule.
3. If the team's settled answer really is "the member slot is always authoritative", then say so where it is enforced and make it visible: the freeze path should overwrite worker.role with snapshot.role in the explicit-selector branch too, instead of preserving a label that is now dead. Right now the code carefully preserves a field that the new precedence guarantees will never be read — that is a trap for the next reader.

Whichever you pick, please add the inverse-direction regression test alongside agent_profile_member_slot_overrides_legacy_role_label. The current test only pins the widening you fixed, so nothing stops the symmetric one from being reintroduced.

Smaller notes

• The role_source value stays "agent_profile.role" in the new branch, which is right, but with the fallthrough gone "task.role" is now only reachable when no member resolved at all. Worth saying that in the comment — it is a meaningful narrowing of what that receipt string means, and receipts are read by people debugging exactly this.
• DCO: 505fc7a27 has no Signed-off-by: trailer (the merge commit e6868d1df does not either). Check Signed-off-by is advisory in .github/workflows/dco.yml, so CI is green, but CONTRIBUTING asks for it — git commit --amend -s.
• No locale concern here; nothing user-visible was added.

Thanks for chasing this one down to a concrete fleet-e12f3160 repro — the diagnosis is right and the comment you wrote explaining the old condition is genuinely better than what it replaced.

[devin-ai-integration]

Devin Review found 2 new potential issues.

[devin-ai-integration]

🟡 Equivalent role aliases block runs

When worker.role uses an equivalent alias, canonical_public_role_name can differ from the selected member’s label. fleet_role_to_agent_type maps pairs like coordinator and manager to one posture. The string comparison rejects these valid runs and case-only custom-role variants.

Suggested change

	let label = canonical_public_role_name(label);
	if label != snapshot.role {
	let label = canonical_public_role_name(label);
	if fleet_role_to_agent_type(Some(&label)) != roster_member_agent_type(profile) {

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

🔍 Changelog updates belong after merge

Repository policy reserves both changelogs for batched updates on main. Remove these PR hunks to avoid conflicts with concurrent work.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[Hmbown]

Replying to @Hmbown's review (requested at the original head e6868d1df): the recovery head reworks the precedence flip into a fail-closed conflict guard, which resolves the mirror-direction widening.

What changed since the reviewed head (freeze_fleet_task_members, crates/tui/src/fleet/worker_runtime.rs): when a task carries both an explicit worker.agent_profile selector and a worker.role label, the guard canonicalizes both names and rejects the task at freeze if they name different postures (conflicting_member_and_role_label_is_rejected_at_freeze_naming_both_postures). Neither side wins — so the mirror case from the review (member:alice slot implement + role: reviewer) no longer widens permissions.write false→true; it never reaches fleet_task_to_worker_spec_with_profiles at all (legacy_role_label_never_widens_into_a_member_write_slot pins that). Aliases of the same posture are still accepted (role_label_alias_of_the_selected_member_role_is_not_a_conflict).

Two deliberate strictness notes:

1. The comparison is on canonical role names, so member:reviewer + role: read-only is rejected even though both are read-only — a task has one posture, and name-equality is the cheapest honest check.
2. When only the legacy role is present and it does not name a member of an explicitly selected Fleet, the task is rejected rather than silently dropped.

The original author's scenario (member:reviewer falling through to the manager label) stays fixed: the resolved member slot still wins over the legacy label — the guard only fires when the two disagree.

CI on the current head (b7ed12fce): Lint, ubuntu, windows, npm wrapper, Version drift, and buildkite #1847 all green; hosted macOS leg still running (Buildkite is the accepted macOS evidence). @Hmbown — the change request predates this guard; if it addresses your concern, a re-review (or dismissal) unblocks the merge.