fix(teams): grant a delegated lead read access to its own team

[yatul]

Fixes #1535. Implements the direction from your triage — give the delegated lead its own team context rather than loosening send_file/message — but through a different mechanism than my first attempt.

Stacked on #1530. The team workspace path is derived from the origin chat, which is exactly what #1530 corrects; based on dev alone the lead would resolve a different directory than its own tasks use. Review only the last commit, or merge #1530 first and this becomes a three-file diff.

The mechanism

My first attempt set RunRequest.TeamWorkspace on the delegated run. That is rejected outright, and deliberately — internal/agent/loop_context.go:233:

if isArtifactDelegation {
    if req.TeamWorkspace != "" ||
        !validateDelegationArtifactWorkspace(req.DelegationID, req.DelegateInputsPath, req.DelegateOutputsPath) {
        return contextSetupResult{}, fmt.Errorf("invalid delegation artifact workspace")
    }

The invariant is right: that field does not only grant read access, it also becomes ToolWorkspace, which would displace the exchange outputs directory. Deployed, every delegation died at setup. This version does not go near the field.

The separation the lead needs already exists in the code. A lead addressed directly resolves its team at loop_context.go:285-321 and gets

ctx = tools.WithToolTeamWorkspace(ctx, wsDir)
ctx = tools.WithToolTeamRoot(ctx, teamRoot)

with no WithToolWorkspace call — a read allowance, not an override. Only the !isArtifactDelegation gate keeps a delegated lead out of it. So the grant is made inside the artifact branch instead, from the same inputs. The guard above stays exactly as strict as it was, and no new field is added to RunRequest.

Both paths are needed

The workspace alone covers only the lead's own chat leaf. The deliverable in the report sits at teams/<id>/system/review-....md — written by a member under a different chat scope. buildAllowedPrefixes adds the team root for reads and not for writes (internal/tools/filesystem.go:366-370), which is precisely the asymmetry this case wants: the lead reads the team's output, per-chat write isolation is untouched.

What is deliberately not granted

ToolTeamID is not set. It switches on the workspace interceptor's write validation, file-change broadcast and task attachment (internal/tools/workspace_interceptor.go:36,114,203) — none of which a delegated run should trigger, and none of which reading needs.

Hermeticity is unchanged: send_file and message still refuse inside an artifact run, and publication still happens through delegation outputs on completion. There is a test pinning that.

The multi-team rule

As requested, ambiguity is not resolved silently: an agent leading more than one active team gets nothing. store.GetTeamForAgent would answer in one call, but it is ORDER BY (lead_agent_id = $1) DESC LIMIT 1 — it would quietly pick a team, which is the choice this must not make on its own.

Tests

property	test
the guard is crossed, outputs not displaced	TestInjectContext_DelegatedLeadReadsTeamWithoutDisplacingOutputs
an agent leading no team gains nothing	TestInjectContext_DelegatedNonLeadGainsNothing
resolution rule: exactly one active led team	SingleLedTeamResolves, MultipleLedTeamsResolveNothing, NonLeadResolvesNothing, InactiveTeamIgnored
shared vs isolated path shape	SharedTeamCollapsesToRoot
no cross-team access	SeparateLeadsGetSeparateWorkspaces
store failure or no store grants nothing	StoreFailureResolvesNothing, NoTeamStoreResolvesNothing
team root widens reads, not writes	TestTeamRootDoesNotWidenWrites
direct sending still blocked	TestSendFileStaysBlockedInDelegationArtifactRun

The first one is the one that matters: my previous attempt had eight green unit tests and still broke every delegation, because not one of them crossed the guard in injectContext. This one drives injectContext itself and asserts both halves — the run is not refused, and ToolWorkspace is still the exchange outputs directory. Reverting the change fails it.

go test ./cmd/... ./internal/tools/ ./internal/agent/... ./internal/gateway/... ./internal/http/... is green.

Verified running, not just under test

Given how the first attempt failed, unit tests alone are not evidence. Built and deployed to a live cluster:

1. the delegation reaches the lead — the previous invalid delegation artifact workspace is gone;
2. the lead reads teams/<id>/system/review-....md, the path from the report, in a chat scope that is not its own — reachable only through the team root;
3. the content comes back verbatim to the caller;
4. separately, the lead reads a team file and writes it into its own outputs/ — the sanctioned publication route the report said was unreachable — byte count matching the source.

Note on the branch

Force-pushed twice: once to replace the first approach, and once to strip 411 built internal/webui/dist assets that rode in from #1530's commit. See the note on #1530. Nothing else moved; the diff is now the three files this change actually touches, plus #1530's.

[yatul]

Converting to draft — this approach is wrong and I found out by deploying it. Flagging before anyone spends review time on it.

Setting RunRequest.TeamWorkspace on a delegated run is explicitly rejected, and deliberately so — internal/agent/loop_context.go:233:

if isArtifactDelegation {
    if req.TeamWorkspace != "" ||
        !validateDelegationArtifactWorkspace(req.DelegationID, req.DelegateInputsPath, req.DelegateOutputsPath) {
        return contextSetupResult{}, fmt.Errorf("invalid delegation artifact workspace")
    }

and the team-workspace application below it is gated on !isArtifactDelegation twice over. In a live cluster every delegation then died at setup:

WARN delegate.async.failed to=brain error="setup context: inject context: invalid delegation artifact workspace"

The invariant makes sense: req.TeamWorkspace does not just grant read access, it also becomes ToolWorkspace, which would displace the artifact outputs directory and break the exchange. My unit tests passed precisely because they exercised the resolver and the allow-list, never this guard — my fault for not covering the wiring end to end.

The idea still seems right; the mechanism has to change. What the lead needs is the read allowance (WithToolTeamWorkspace, feeding allowedWithTeamWorkspace) without the workspace override — so it cannot travel through req.TeamWorkspace as long as that field carries both meanings.

Two shapes, and since the guard is deliberate I would rather you pick:

1. A separate field, e.g. RunRequest.TeamReadWorkspace, applied inside the artifact branch as a read allowance only. Leaves the existing invariant and its guard untouched.
2. Split the meaning of TeamWorkspace — keep rejecting it as a workspace override in artifact runs, but let the artifact branch adopt it for reads. Fewer fields, but weakens a guard that currently says something simple.

I lean towards (1). Happy to push it once you say which.

Sorry for the noise — should have caught this before opening.

[clark-cant]

Review: fix(teams): give a delegated lead its own team workspace

Summary: Resolves #1535 by populating the delegated lead's TeamWorkspace from its own team context, restoring read access to team files that was always available to team members but missing from the agent-link delegation path. Stacked on #1530 (already merged).

Risk level: Low — focused Go changes (+450/-16 across 14 source files) with comprehensive regression tests. The 400+ webui dist files are an unrelated UI rebuild bundled in.

Mandatory gates:

• Duplicate / prior implementation: clear — no overlapping PR found; #1530 (prerequisite) already merged
• Project standards: aligned — uses existing patterns (ListTeams walk, allowedWithTeamWorkspace validation, OriginChannelFromCtx helpers)
• Strategic necessity: clear value — completes the delegated-lead capability chain (dispatch → routing → workspace access)

Findings:

• Suggestion: The PR bundles ~400 webui dist asset files unrelated to the Go fix. Consider separating UI rebuilds into dedicated commits/PRs for cleaner history and easier bisect.
• Suggestion: The multi-team rule (agent leading >1 team gets no workspace) is correct defensive behavior. Worth documenting this constraint in team architecture docs.

Verdict: Approve — well-designed narrow fix with proper test coverage (8 test cases covering resolution, isolation, hermeticity). No Critical or Important findings.

Posted by /ck:review-pr at 2026-08-31T20:44:00Z

[yatul]

Ready for review — reworked and out of draft.

I put this in draft on 27 August because the first approach was wrong: it set RunRequest.TeamWorkspace on the delegated run, which injectContext rejects by design, and every delegation died at setup. I asked which of two shapes you preferred — a new TeamReadWorkspace field, or splitting the meaning of TeamWorkspace.

Neither turned out to be necessary, so I did not wait for the answer. The read-allowance-without-override separation already exists in the code: a lead addressed directly gets WithToolTeamWorkspace + WithToolTeamRoot and no WithToolWorkspace (loop_context.go:285-321). Only the !isArtifactDelegation gate excluded a delegated lead. The grant now happens inside the artifact branch from the same inputs, the guard on req.TeamWorkspace is untouched, and RunRequest gains no field. The PR body has the details.

Two things worth your attention:

The team root is not optional. The workspace alone covers only the lead's own chat leaf, and the deliverable in the report lives under another member's chat scope. buildAllowedPrefixes already adds the root for reads and not for writes, so this grants exactly the asymmetry the case needs.

ToolTeamID is deliberately left unset, since it would switch on the workspace interceptor's write validation and broadcast, which a delegated run has no business triggering.

I also verified it running, not only under test — deployed and drove a real delegation end to end, including the lead reading the exact path from the report and staging a file into its delegation outputs. My previous attempt passed eight unit tests and still broke everything, so I no longer treat green tests as evidence here. There is now a test that crosses the guard those eight missed.

[yatul]

Flagging that the approval on this PR is stale, rather than quietly merging on it.

The review was submitted on 31 August against 019cea10. The branch head is now 91376643, and the approval carried across the force-push. Three details in the review text confirm it describes the earlier state and not the current one:

• "populating the delegated lead's TeamWorkspace from its own team context" — that is the first approach, which I withdrew. Setting RunRequest.TeamWorkspace on a delegated run is rejected by injectContext by design, because that field also becomes ToolWorkspace and would displace the exchange outputs directory. Deployed, it killed every delegation at setup.
• "The 400+ webui dist files are an unrelated UI rebuild bundled in" — those were build output that rode in from fix(teams): scope team tasks to the delegation origin, not the delivery channel #1530's commit. Both branches were force-pushed to strip them; the diff is now 14 files, all source.
• "Stacked on fix(teams): scope team tasks to the delegation origin, not the delivery channel #1530 (already merged)" — fix(teams): scope team tasks to the delegation origin, not the delivery channel #1530 is still open. This PR still depends on it.

What is on the branch now takes a different route: the read-allowance-without-override separation already exists in loop_context.go:285-321, where a directly addressed lead gets WithToolTeamWorkspace + WithToolTeamRoot and no WithToolWorkspace. Only the !isArtifactDelegation gate excluded a delegated lead, so the grant is made inside the artifact branch from the same inputs. The guard on req.TeamWorkspace is untouched and RunRequest gains no field. Full reasoning is in the PR body, which I rewrote at the same time; I have also corrected the PR title, which still described the withdrawn approach.

Re-review should be cheap — the whole change is three source files plus tests, and the one that matters is TestInjectContext_DelegatedLeadReadsTeamWithoutDisplacingOutputs, which drives injectContext through the guard the first attempt tripped over.

I would rather this went in on a review of the code that is actually here.