From fee439d75caf916adcc2890b761042fb32ca8561 Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 6 Aug 2026 14:14:59 -0400 Subject: [PATCH] feat(hooks): report the pull-request author in lifecycle payloads Hook payloads identified the pull request only by URL, so a notifier that wanted to address the author had to re-fetch the pull request from the host on every verdict, in a path that is meant to cost nothing. Add `author`, the author's git-host login, plus the matching `CR_AUTHOR` environment variable. The value is observed from the pull-request snapshot every run already reads, so it costs no extra host call and covers review, respond, and gate early-exit alike. The first non-empty login wins: a run reads the pull request repeatedly, and a later unauthored snapshot must not erase an identity the hooks already reported. Because the observation rides the provider progress wrapper, that wrapper no longer requires a progress logger; span creation is now nil-safe so a profile with hooks and no logger still observes provider calls. Closes #554 --- README.md | 11 ++++-- internal/app/hook_dispatch.go | 18 ++++++++++ internal/app/hook_dispatch_test.go | 55 +++++++++++++++++++++++++++++- internal/app/provider_progress.go | 52 ++++++++++++++++++---------- internal/app/runtime.go | 4 +-- internal/hooks/hooks.go | 10 ++++-- internal/hooks/hooks_test.go | 9 ++--- 7 files changed, 128 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index ef4cd887..332ac647 100644 --- a/README.md +++ b/README.md @@ -607,13 +607,18 @@ hooks. Every payload contains `event`, `pr_url`, `run_id`, `profile`, `pass_number` (the ledger attempt), `artifact_dir`, and `dry_run`; terminal events also contain -`outcome`. `reviewer.completed` adds `reviewer_id` and `reviewer_status`, +`outcome`. `author` carries the pull request author's git-host login (GitHub +login, GitLab username) from the moment the run reads the pull request, which is +every event after `run.started`, and is omitted when a run fails before that +read. It is observed from the snapshot the run already fetches, so a hook that +addresses the author costs no extra host call. `reviewer.completed` adds +`reviewer_id` and `reviewer_status`, `posting.action` adds `action_kind` and the canonical `action_marker` when the provider call carries one, and `selection.completed` adds sorted `agents` plus an agent-to-model `models` object. Values not yet allocated at an early event, such as the run ID at `run.started`, are empty. The common fields are also -available as `CR_EVENT`, `CR_PR_URL`, `CR_RUN_ID`, `CR_OUTCOME`, `CR_PROFILE`, -`CR_PASS_NUMBER`, `CR_ARTIFACT_DIR`, and `CR_DRY_RUN`. +available as `CR_EVENT`, `CR_PR_URL`, `CR_RUN_ID`, `CR_AUTHOR`, `CR_OUTCOME`, +`CR_PROFILE`, `CR_PASS_NUMBER`, `CR_ARTIFACT_DIR`, and `CR_DRY_RUN`. Missing commands, non-zero exits, and timeouts write warnings to stderr with combined command output capped at 8 KiB. They never change the pipeline result diff --git a/internal/app/hook_dispatch.go b/internal/app/hook_dispatch.go index d35f67cf..8c36a034 100644 --- a/internal/app/hook_dispatch.go +++ b/internal/app/hook_dispatch.go @@ -27,6 +27,7 @@ type hookDispatcher struct { mu sync.Mutex dryRun bool + author string run ledger.Run once map[string]bool selectionSeen bool @@ -83,6 +84,7 @@ func (d *hookDispatcher) emit(event string, extra hooks.Payload, run ledger.Run, Event: event, PRURL: d.prURL, RunID: run.RunID, + Author: d.author, Profile: d.profile, PassNumber: run.Attempt, ArtifactDir: run.ArtifactPath, @@ -134,6 +136,22 @@ func (d *hookDispatcher) observeRunID(runID string) ledger.Run { return run } +// observeAuthor records the pull request author for every later event. The +// first non-empty login wins: a run reads the pull request repeatedly, and a +// later read that fails or returns an unauthored snapshot must not erase an +// identity the hooks already reported. +func (d *hookDispatcher) observeAuthor(login string) { + login = strings.TrimSpace(login) + if d == nil || login == "" { + return + } + d.mu.Lock() + defer d.mu.Unlock() + if d.author == "" { + d.author = login + } +} + func (d *hookDispatcher) observeRun(run ledger.Run) { if d == nil || run.RunID == "" { return diff --git a/internal/app/hook_dispatch_test.go b/internal/app/hook_dispatch_test.go index 1b0a8e61..152152e5 100644 --- a/internal/app/hook_dispatch_test.go +++ b/internal/app/hook_dispatch_test.go @@ -58,6 +58,14 @@ func TestReviewHooksFanOutFromExistingProgressSeams(t *testing.T) { PRURL: "https://github.com/acme/repo/pull/7", }, store) dispatcher.begin(false) + ref := gitprovider.PRRef{Host: "github.com", Owner: "acme", Repo: "repo", Number: 7} + fake := &gitprovider.Fake{} + if err := fake.SetPR(ref, gitprovider.PR{Ref: ref, State: gitprovider.PRStateOpen, Author: gitprovider.Identity{Login: "piekstra", ID: "1795"}}); err != nil { + t.Fatalf("SetPR: %v", err) + } + if _, err := withProgressProvider(nil, dispatcher, "review", fake).GetPR(context.Background(), ref); err != nil { + t.Fatalf("GetPR: %v", err) + } progress := newPipelineTaskProgress(nil, "review", dispatcher) logPath := run.ArtifactPath + "/agent-logs/task.jsonl" @@ -84,7 +92,6 @@ func TestReviewHooksFanOutFromExistingProgressSeams(t *testing.T) { t.Fatalf("RenderAction: %v", err) } provider := withHookProvider(dispatcher, &gitprovider.Fake{}) - ref := gitprovider.PRRef{Host: "github.com", Owner: "acme", Repo: "repo", Number: 7} if _, err := provider.PostIssueComment(context.Background(), ref, rendered+"\n\nreview"); err != nil { t.Fatalf("PostIssueComment: %v", err) } @@ -113,6 +120,52 @@ func TestReviewHooksFanOutFromExistingProgressSeams(t *testing.T) { if got := byEvent["posting.action"][0]; got.ActionKind != marker.ActionKindRollupComment || got.ActionMarker != rendered { t.Fatalf("posting payload = %#v", got) } + if got := byEvent["run.started"][0]; got.Author != "" { + t.Fatalf("run.started carried an author before the pull request was read: %#v", got) + } + for _, event := range []string{"workspace.prepared", "dossier.ready", "selection.completed", "reviewer.completed", "plan.ready", "posting.action", "run.completed"} { + if got := byEvent[event][0]; got.Author != "piekstra" { + t.Fatalf("event %s author = %q, want piekstra", event, got.Author) + } + } +} + +func TestHookAuthorKeepsTheFirstIdentityReadFromTheProvider(t *testing.T) { + capture := t.TempDir() + "/events.jsonl" + t.Setenv("GO_WANT_APP_HOOK_HELPER", "1") + t.Setenv("APP_HOOK_CAPTURE", capture) + dispatcher := newHookDispatcher(OpenRequest{ + Profile: config.Profile{Hooks: hookEntries([]string{"run.completed"})}, ProfileName: "work", Command: "review", + }, hookStore{}) + ref := gitprovider.PRRef{Host: "github.com", Owner: "acme", Repo: "repo", Number: 7} + fake := &gitprovider.Fake{} + provider := withProgressProvider(nil, dispatcher, "review", fake) + for _, pr := range []gitprovider.PR{ + {Ref: ref, State: gitprovider.PRStateOpen, Author: gitprovider.Identity{Login: "piekstra"}}, + {Ref: ref, State: gitprovider.PRStateOpen}, + } { + if err := fake.SetPR(ref, pr); err != nil { + t.Fatalf("SetPR: %v", err) + } + if _, err := provider.GetPR(context.Background(), ref); err != nil { + t.Fatalf("GetPR: %v", err) + } + } + dispatcher.emit("run.completed", hooks.Payload{Outcome: "approved"}, ledger.Run{RunID: "run-1"}, true) + dispatcher.drain() + + payloads := readAppHookPayloads(t, capture) + if len(payloads) != 1 || payloads[0].Author != "piekstra" { + t.Fatalf("payloads = %#v, want one carrying author piekstra", payloads) + } +} + +func TestHookProviderWrapperIsSkippedWhenNoHooksAreConfigured(t *testing.T) { + dispatcher := newHookDispatcher(OpenRequest{ProfileName: "work", Command: "review"}, hookStore{}) + fake := &gitprovider.Fake{} + if got := withProgressProvider(nil, dispatcher, "review", fake); got != gitprovider.GitProvider(fake) { + t.Fatalf("provider = %#v, want the unwrapped provider", got) + } } func TestRespondHookNamespaceDoesNotFireReviewEvents(t *testing.T) { diff --git a/internal/app/provider_progress.go b/internal/app/provider_progress.go index 25b965f5..3d40555d 100644 --- a/internal/app/provider_progress.go +++ b/internal/app/provider_progress.go @@ -13,6 +13,7 @@ import ( type progressProvider struct { provider gitprovider.GitProvider logger *progress.Logger + hooks *hookDispatcher command string } @@ -20,18 +21,21 @@ type progressRangeProvider struct { progressProvider } -func withProgressProvider(logger *progress.Logger, command string, provider gitprovider.GitProvider) gitprovider.GitProvider { +func withProgressProvider(logger *progress.Logger, dispatcher *hookDispatcher, command string, provider gitprovider.GitProvider) gitprovider.GitProvider { if provider == nil { return nil } - if logger == nil { + if dispatcher != nil && !dispatcher.enabled { + dispatcher = nil + } + if logger == nil && dispatcher == nil { return provider } command = strings.TrimSpace(command) if command == "" { command = "review" } - wrapped := progressProvider{provider: provider, logger: logger, command: command} + wrapped := progressProvider{provider: provider, logger: logger, hooks: dispatcher, command: command} if _, ok := provider.(interface { GetDiffBetweenRefs(context.Context, gitprovider.PRRef, string, string) (gitprovider.UnifiedDiff, error) }); ok { @@ -40,26 +44,38 @@ func withProgressProvider(logger *progress.Logger, command string, provider gitp return wrapped } +// start opens a progress span, or none when the wrapper exists only to observe +// provider calls for hooks. +func (p progressProvider) start(op, target string) *progress.Span { + if p.logger == nil { + return nil + } + return p.logger.Start(p.command, op, target) +} + func (p progressProvider) WhoAmI(ctx context.Context, creds gitprovider.Credential) (gitprovider.Identity, error) { - span := p.logger.Start(p.command, "resolve_identity", "runtime") + span := p.start("resolve_identity", "runtime") identity, err := p.provider.WhoAmI(ctx, creds) return identity, endProgressSpan(span, err) } func (p progressProvider) ReviewAuthority(ctx context.Context, ref gitprovider.PRRef, identity gitprovider.Identity) (gitprovider.ReviewAuthority, error) { - span := p.logger.Start(p.command, "check_review_authority", "runtime") + span := p.start("check_review_authority", "runtime") authority, err := p.provider.ReviewAuthority(ctx, ref, identity) return authority, endProgressSpan(span, err) } func (p progressProvider) GetPR(ctx context.Context, ref gitprovider.PRRef) (gitprovider.PR, error) { - span := p.logger.Start(p.command, "fetch_pr", "pr") + span := p.start("fetch_pr", "pr") pr, err := p.provider.GetPR(ctx, ref) + if err == nil && p.hooks != nil { + p.hooks.observeAuthor(pr.Author.Login) + } return pr, endProgressSpan(span, err) } func (p progressProvider) GetDiff(ctx context.Context, ref gitprovider.PRRef) (gitprovider.UnifiedDiff, error) { - span := p.logger.Start(p.command, "fetch_diff", "pr") + span := p.start("fetch_diff", "pr") diff, err := p.provider.GetDiff(ctx, ref) return diff, endProgressSpan(span, err) } @@ -68,66 +84,66 @@ func (p progressRangeProvider) GetDiffBetweenRefs(ctx context.Context, ref gitpr rangeProvider := p.provider.(interface { GetDiffBetweenRefs(context.Context, gitprovider.PRRef, string, string) (gitprovider.UnifiedDiff, error) }) - span := p.logger.Start(p.command, "fetch_diff_between_refs", "pr") + span := p.start("fetch_diff_between_refs", "pr") diff, err := rangeProvider.GetDiffBetweenRefs(ctx, ref, baseSHA, headSHA) return diff, endProgressSpan(span, err) } func (p progressProvider) GetFileAtRef(ctx context.Context, ref gitprovider.PRRef, gitRef, path string) ([]byte, error) { - span := p.logger.Start(p.command, "read_file", fileTarget(path)) + span := p.start("read_file", fileTarget(path)) data, err := p.provider.GetFileAtRef(ctx, ref, gitRef, path) return data, endProgressSpan(span, err) } func (p progressProvider) ListTreeAtRef(ctx context.Context, ref gitprovider.PRRef, gitRef, path string) ([]gitprovider.TreeEntry, error) { - span := p.logger.Start(p.command, "list_tree", fileTarget(path)) + span := p.start("list_tree", fileTarget(path)) entries, err := p.provider.ListTreeAtRef(ctx, ref, gitRef, path) return entries, endProgressSpan(span, err) } func (p progressProvider) ListInlineThreads(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.InlineThread, error) { - span := p.logger.Start(p.command, "list_threads", "threads") + span := p.start("list_threads", "threads") threads, err := p.provider.ListInlineThreads(ctx, ref) return threads, endProgressSpan(span, err) } func (p progressProvider) ListReviews(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.Review, error) { - span := p.logger.Start(p.command, "list_reviews", "reviews") + span := p.start("list_reviews", "reviews") reviews, err := p.provider.ListReviews(ctx, ref) return reviews, endProgressSpan(span, err) } func (p progressProvider) ListIssueComments(ctx context.Context, ref gitprovider.PRRef) ([]gitprovider.IssueComment, error) { - span := p.logger.Start(p.command, "list_issue_comments", "posts") + span := p.start("list_issue_comments", "posts") comments, err := p.provider.ListIssueComments(ctx, ref) return comments, endProgressSpan(span, err) } func (p progressProvider) PostInlineComment(ctx context.Context, ref gitprovider.PRRef, c gitprovider.InlineComment) (gitprovider.CommentID, error) { - span := p.logger.Start(p.command, "post_inline_comment", "posts") + span := p.start("post_inline_comment", "posts") id, err := p.provider.PostInlineComment(ctx, ref, c) return id, endProgressSpan(span, err) } func (p progressProvider) ReplyToThread(ctx context.Context, ref gitprovider.PRRef, threadID gitprovider.ThreadID, body string) (gitprovider.CommentID, error) { - span := p.logger.Start(p.command, "reply_thread", "posts") + span := p.start("reply_thread", "posts") id, err := p.provider.ReplyToThread(ctx, ref, threadID, body) return id, endProgressSpan(span, err) } func (p progressProvider) ResolveThread(ctx context.Context, ref gitprovider.PRRef, threadID gitprovider.ThreadID) error { - span := p.logger.Start(p.command, "resolve_thread", "posts") + span := p.start("resolve_thread", "posts") return endProgressSpan(span, p.provider.ResolveThread(ctx, ref, threadID)) } func (p progressProvider) PostIssueComment(ctx context.Context, ref gitprovider.PRRef, body string) (gitprovider.CommentID, error) { - span := p.logger.Start(p.command, "post_issue_comment", "posts") + span := p.start("post_issue_comment", "posts") id, err := p.provider.PostIssueComment(ctx, ref, body) return id, endProgressSpan(span, err) } func (p progressProvider) SubmitReview(ctx context.Context, ref gitprovider.PRRef, r gitprovider.ReviewRequest) (gitprovider.ReviewID, error) { - span := p.logger.Start(p.command, "submit_review", "posts") + span := p.start("submit_review", "posts") id, err := p.provider.SubmitReview(ctx, ref, r) return id, endProgressSpan(span, err) } diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 0eb54c9c..3d760fb0 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -173,7 +173,7 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { cleanup() return Runtime{}, err } - repoProvider = withProgressProvider(req.Progress, command, repoProvider) + repoProvider = withProgressProvider(req.Progress, dispatcher, command, repoProvider) postingGit := gitConfigForReviewerAuth(profile) _, postingProviderStore, err := stores.Open(postingGit.Credential) if err != nil { @@ -186,7 +186,7 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) { return Runtime{}, err } rawPostingProvider := postingProvider - postingProvider = withProgressProvider(req.Progress, command, postingProvider) + postingProvider = withProgressProvider(req.Progress, dispatcher, command, postingProvider) postingProvider = withHookProvider(dispatcher, postingProvider) postingIdentity, err := deps.ResolvePostingIdentity(ctx, postingProvider, credential, postingProviderStore, profile) if err != nil { diff --git a/internal/hooks/hooks.go b/internal/hooks/hooks.go index 25a60fd2..52294b78 100644 --- a/internal/hooks/hooks.go +++ b/internal/hooks/hooks.go @@ -23,9 +23,12 @@ const maxOutput = 8 << 10 // set only for terminal events. The remaining optional fields are specific to // selection, reviewer, and posting events. type Payload struct { - Event string `json:"event"` - PRURL string `json:"pr_url"` - RunID string `json:"run_id"` + Event string `json:"event"` + PRURL string `json:"pr_url"` + RunID string `json:"run_id"` + // Author is the pull request author's git-host login, known once the run + // has read the pull request. + Author string `json:"author,omitempty"` Outcome string `json:"outcome,omitempty"` Profile string `json:"profile"` PassNumber int `json:"pass_number"` @@ -130,6 +133,7 @@ func payloadEnv(payload Payload) []string { "CR_EVENT=" + payload.Event, "CR_PR_URL=" + payload.PRURL, "CR_RUN_ID=" + payload.RunID, + "CR_AUTHOR=" + payload.Author, "CR_OUTCOME=" + payload.Outcome, "CR_PROFILE=" + payload.Profile, "CR_PASS_NUMBER=" + strconv.Itoa(payload.PassNumber), diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index 3aa0706f..0de2f794 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -41,8 +41,8 @@ func TestHookHelperProcess(_ *testing.T) { // #nosec G304,G703 -- the parent test supplies a t.TempDir capture path. _ = os.WriteFile(path, []byte(strings.Join([]string{ os.Getenv("CR_EVENT"), os.Getenv("CR_PR_URL"), os.Getenv("CR_RUN_ID"), - os.Getenv("CR_OUTCOME"), os.Getenv("CR_PROFILE"), os.Getenv("CR_PASS_NUMBER"), - os.Getenv("CR_ARTIFACT_DIR"), os.Getenv("CR_DRY_RUN"), + os.Getenv("CR_AUTHOR"), os.Getenv("CR_OUTCOME"), os.Getenv("CR_PROFILE"), + os.Getenv("CR_PASS_NUMBER"), os.Getenv("CR_ARTIFACT_DIR"), os.Getenv("CR_DRY_RUN"), }, "\n")), 0o600) } if code, _ := strconv.Atoi(os.Getenv("HOOK_HELPER_EXIT")); code != 0 { @@ -60,7 +60,8 @@ func TestDispatcherWritesPayloadAndCommonEnvironment(t *testing.T) { dispatcher := New([]config.Hook{{Event: "run.completed", Argv: helperArgv(), Timeout: "1s"}}, io.Discard) want := Payload{ Event: "run.completed", PRURL: "https://github.com/acme/repo/pull/7", RunID: "run-7", - Outcome: "approved", Profile: "work", PassNumber: 2, ArtifactDir: "/tmp/run-7", DryRun: false, + Author: "piekstra", Outcome: "approved", Profile: "work", PassNumber: 2, + ArtifactDir: "/tmp/run-7", DryRun: false, } dispatcher.Dispatch(want) dispatcher.Drain() @@ -74,7 +75,7 @@ func TestDispatcherWritesPayloadAndCommonEnvironment(t *testing.T) { if err != nil { t.Fatalf("read env: %v", err) } - wantEnv := "run.completed\nhttps://github.com/acme/repo/pull/7\nrun-7\napproved\nwork\n2\n/tmp/run-7\nfalse" + wantEnv := "run.completed\nhttps://github.com/acme/repo/pull/7\nrun-7\npiekstra\napproved\nwork\n2\n/tmp/run-7\nfalse" if string(envBody) != wantEnv { t.Fatalf("env = %q, want %q", envBody, wantEnv) }