Repository navigation
Artifact view: link back to the session's channels (PR + Slack thread) - #32
josephschorr wants to merge 14 commits into
Conversation
…rived Deletes the BackLink field from passthroughlink.Payload and drops the backLink parameter from the MintArtifactViewLink/MintArtifactDownloadLink/ MintSessionViewLink interfaces and every implementation, plus the click-time GetPermalinkContext lookup in the Slack listener. Origin links now come from the session bindings via OriginLinks, not the signed link.
…sions Review found that the sender captures a canonical permalink only on bot-rooted first-sends (needsWriteBack); a human-initiated Slack session's thread predates the agent, so it never got one and its artifact view lost the back-to-thread link (a regression). OriginLink now falls back to BuildSlackThreadURL from the team_id/channel_id/thread_ts the listener stamps at creation — symmetric with github's key reconstruction. Adds the no-provider-kind test case too.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…l_request body - relay.go: clarify the permalink is not backfilled for the same thread root (the thread_ts guard blocks re-patch); the view falls back to a reconstructed link. - github TriggerOriginURL: treat an empty body as 'nothing to derive' (ok=false, no error) instead of logging a spurious decode error.
| // "Back to origin" links derived from the session's channel bindings. A | ||
| // best-effort lookup (OriginLinks never errors), coerced to a non-nil | ||
| // slice so the bootstrap JSON is always an array the client can map over. | ||
| originLinks := av.OriginLinks(ctx, ns, sess) |
There was a problem hiding this comment.
OriginLinks (Slack thread/DM permalink, private PR URL, from both input and output bindings) go to every subject that passes CheckView, not only session participants. The page never gates them on CheckInteract the way bindView.mirrorsAllowed gates the other session-scoped mirrors.
A session has opted into artifact_org_viewer, or the viewer is a platform admin (view = parent->interact + parent->artifact_org_view + platform->view_audit). Any org user opening the durable /artifact-view?artifactId=..&sessionRef=.. URL then gets the private Slack channel/DM thread permalink (team/channel IDs, the fact that the DM exists) and the private-repo PR URL in the bootstrap JSON. Before this change the durable path carried backLink="" and the signed path only carried a back link minted for the Slack clicker who was already in the thread. Fix: compute origin links only when b.mirrorsAllowed(ctx, av, subject) is true, and return [] otherwise.
There was a problem hiding this comment.
Gated the origin links behind mirrorsAllowed (CheckInteract) in shellPageBuild, same as the other session mirrors — a viewer who passes CheckView but is not a session participant now gets originLinks: []. Added TestShell_OriginLinksGatedForNonParticipant.
| if binding == nil { | ||
| return "", false | ||
| } | ||
| if u := binding.External["permalink"]; u != "" { |
There was a problem hiding this comment.
OriginLink prefers External["permalink"] unconditionally, but a branching restart fork (forkChildBinding in pkg/controllers/agentsession/restart_decide.go) strips only thread_ts and copies permalink. The relay merge-patch also never deletes a stale permalink when the new root's getPermalink fails.
A parent session's binding has permalink=P (parent thread). A user runs restart-from-here, and the child gets External{team_id,channel_id,permalink:P} with no thread_ts. Until the child's first send, and permanently if chat.getPermalink fails on that send (the merge patch writes thread_ts but leaves permalink:P), the child's artifact view links to the PARENT thread, a different conversation, so viewers are sent to the wrong thread. Fix: strip permalink alongside thread_ts in forkChildBinding, and have patchOutputChannel/patchInputChannelThreadTS write permalink:null when none was captured (or only trust permalink when it was captured with the current thread_ts).
There was a problem hiding this comment.
Fixed in forkChildBinding: it now strips permalink alongside thread_ts on a branching fork, so the child no longer carries the parent thread's permalink. I did not add the patch-func permalink:null write — once the fork drops it, the child re-captures its own permalink on first send (or OriginLink reconstructs from the new thread_ts), so no stale value is left to clear. Covered by the fork tests (strip on branch, keep on inherit).
| if err != nil { | ||
| return "", false | ||
| } | ||
| return fmt.Sprintf("https://github.com/%s/%s/pull/%d", owner, repo, number), true |
There was a problem hiding this comment.
When origin_url is absent, OriginLink falls back to a hardcoded https://github.com///pull/, even on GitHub Enterprise Server installs that this kind explicitly supports.
On a GHES install, any session created before origin_url was persisted, or from a delivery without html_url, renders a 'View the pull request' link to github.com///pull/N. Clicking it sends the internal org/repo names to a third party. If someone has registered the same owner/repo on github.com, the trusted toolbar sends viewers to attacker-controlled content. Fix: derive the host from the Channel's configured GHES base URL, or return ok=false for the fallback when the host is unknown.
There was a problem hiding this comment.
Removed the github.com fallback — OriginLink now returns ok=false when origin_url is absent. The GHES host is not derivable from the binding (the base URL lives on the RelationshipSource spec, not here), so fail-closed is the safe choice; new sessions always persist origin_url.
| return | ||
| } | ||
| seen[u] = true | ||
| out = append(out, artifactview.OriginLink{Href: u, ChannelKind: b.Kind}) |
There was a problem hiding this comment.
Origin link hrefs come straight from spec.{input,output}Channel.external (user-writable on AgentSession) and are rendered as with no scheme or host validation.
A user who can create or patch an AgentSession sets inputChannel{kind: github, external: {origin_url: "https://evil.example/login"}} (or a javascript: URL). Every artifact viewer of that session sees a trusted 'View the pull request' GitHub icon pointing there. The app uses React 18, which does not block javascript: hrefs; today only the nonce-based script-src CSP prevents XSS. Fix: in originLinksFrom (or in each provider), require https and, for slack, a *.slack.com host (and for github, the configured GitHub host).
There was a problem hiding this comment.
Added validation: a generic https backstop in originLinksFrom, a *.slack.com host check in the slack kind, and https in the github kind. The per-kind checks live in the kind (not webd) to keep kind logic out of the consumer. github cannot be host-allowlisted (arbitrary GHES hosts), so it is scheme-only.
| // origin" link. The permalink is resolved here because only the sender holds a | ||
| // Slack client. A getPermalink failure is logged and the permalink omitted — it | ||
| // must never block the send, exactly as the former click-time lookup did. | ||
| func (s *slackSender) threadOriginExternal(ctx context.Context, logger interface{ Info(string, ...any) }, channelID, rootTS string) map[string]string { |
There was a problem hiding this comment.
Only the user_message chat.postMessage and streaming-bubble paths resolve a permalink. The other thread-rooting paths still return External without one: the attachment root (sender.go ~792), the fork-root plus attachments path (~291), and the tool_session bubble (sender_tool_session.go:345). The relay patches only once per thread root.
A reviewbot-style session (github input, Slack output) whose first output is a tool_session bubble or an attachment upload roots the thread without a permalink. The relay's thread_ts guard then blocks every later patch. The output binding has no team_id (OutboundAnchor sets only channel_id), so the Slack fallback can't rebuild a link, and the artifact view never shows the Slack origin. Fix: route every first-root writeback through threadOriginExternal.
There was a problem hiding this comment.
Lifted threadOriginExternal to a shared package func and routed all three first-root paths through it — the attachment root (~792), fork-root+attachments (~291), and the tool_session bubble (sender_tool_session.go:345). For tool_session the getPermalink runs once, on the first apply, outside ref.mu. Added TestSender_AttachmentFirstRoot_CapturesPermalink.
| const tip = | ||
| channelKind === "slack" | ||
| ? "Open the Slack thread" | ||
| : channelKind === "github" |
There was a problem hiding this comment.
The artifactview UI now branches on channelKind === "github" (and "slack") for the tooltip and icon (ThreadLink.tsx, icons.tsx), outside the kind's own package.
CLAUDE.md (repo root): "If you're writing if kind == "slack" (or equivalent) outside the kind's own package, the abstraction is missing a method — fix the interface, don't add a switch in the consumer." Every new OriginLinkProvider kind will need another edit here. Fix: have OriginLinkProvider return a label (and icon key) alongside the URL and carry it on artifactview.OriginLink.
There was a problem hiding this comment.
OriginLinkProvider.OriginLink now returns a Label and an Icon key, carried on artifactview.OriginLink. ThreadLink renders label directly and icons.tsx maps the icon key off a channelGlyphs table — no more channelKind === branching. The residual key→SVG map is a glyph vocabulary (a React glyph cannot come from Go), not a kind switch.
|
|
||
| // OriginLinks implements artifactview.Deps: it loads the session and derives its | ||
| // origin links. A Get failure logs and returns no links — never an error, the | ||
| // toolbar simply shows nothing — matching ChannelKind's best-effort contract. |
There was a problem hiding this comment.
Stale doc comment: it says the behavior is 'matching ChannelKind's best-effort contract', but this PR deleted ChannelKind.
A reader looks for a ChannelKind method that no longer exists. Reword it to describe the contract directly.
There was a problem hiding this comment.
Reworded — dropped the dangling ChannelKind reference and described the best-effort contract directly.
…link capture Addresses the review on PR #32: - Gate the artifact-view origin links behind CheckInteract (mirrorsAllowed), not just CheckView, so a platform admin or org-viewer who is not a session participant no longer receives the private Slack permalink or private-repo PR URL. (page.go) - Strip permalink alongside thread_ts when a branching restart fork creates a child: the parent-thread permalink would otherwise point a child's origin link at the wrong conversation. (restart_decide.go) - Remove the hardcoded github.com fallback in github OriginLink; return ok=false when origin_url is absent. The host is unknowable from the binding on GHES, and a reconstructed github.com URL would leak internal org/repo names or point at a squatted repo. (github/kind.go) - Validate origin hrefs: require https (a generic backstop in originLinksFrom), require a *.slack.com host for Slack permalinks, and https for github — the External map is user-writable and rendered as <a href>. Per-kind checks live in the kind. (origin_links.go, slack/kind.go, github/kind.go) - Capture a permalink on the remaining first-root Slack paths — attachment root, fork-root+attachments, and the tool_session bubble — by routing them through a shared threadOriginExternal. Without this a reviewbot-style session whose first output is an attachment or tool session never shows a Slack origin (the output binding has no team_id to reconstruct from). (sender.go, sender_tool_session.go) - Carry Label and Icon on OriginLink so the artifact-view UI renders them off a key->glyph map instead of branching on channelKind, per the no-"if kind==" rule. (channelkinds/kind.go, deps.go, ThreadLink.tsx, icons.tsx, types.ts) - Reword the stale doc comment referencing the deleted ChannelKind type. (origin_links.go) Regenerates the artifact-view webassets bundle.
What
The artifact-view page now links back to every channel a session is attached to — the GitHub pull request it was triggered from and the Slack thread it was delivered to — instead of a single link.
Why
reviewbot's review artifact didn't link back to its Slack thread. The old back-link was only set when someone clicked "View live" in Slack, but reviewbot is triggered by a GitHub webhook (no human click), so nothing was ever populated.
How
OriginLinkProviderchannel-kind interface (one link per distinct origin, de-duped).