feat: add experimental Jira poll input driver - #5778
Conversation
Implement a Jira poll input driver per ADR 0063, enabling fullsend agents to be triggered from Jira issue events (comments, label changes, status transitions) via `fullsend poll --input-driver jira-poll`. - internal/forge/jira/: Jira Cloud REST API v3 client with dual auth (Basic/Bearer), retry with backoff, and project role membership lookup - internal/jirapoll/: Poll engine with write-then-verify lock coordination via Jira entity properties, change detection, ADF text extraction, NormalizedEvent conversion per jira-poll-adapter spec, and actor role resolution from Jira project roles - internal/cli/poll.go: CLI wiring with --input-driver jira-poll flag and Jira-specific flags (--jira-url, --jira-project, --jql, --target-repo) - docs/plans/jira-poll-input-driver.md: Implementation plan with search/jql migration notes and OAuth 2.0 client credentials plan - e2e/behaviour/features/dispatch/jira-poll-dispatch.feature: Gherkin scenarios for mock-based behaviour testing Known issues documented in plan: - search/jql API needs cursor pagination (nextPageToken) and expand as string not array - OAuth 2.0 client credentials auth needed for managed Atlassian instances that restrict personal API tokens Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Users need an OAuth 2.0 service account (OpenID client credentials) from their Atlassian org admin — personal API tokens are typically restricted on managed instances and will not work. Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add NewOAuth2 constructor and oauth2TokenSource to support OAuth 2.0 two-legged (client credentials) auth as an alternative to Basic/Bearer auth with API tokens. The token source caches access tokens and refreshes automatically 5 minutes before expiry. CLI wiring reads JIRA_AUTH_METHOD to select between oauth2 (JIRA_CLIENT_ID + JIRA_CLIENT_SECRET) and the existing Basic/Bearer path (JIRA_TOKEN + JIRA_USER_EMAIL). Default behavior is unchanged. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
The POST /rest/api/3/search/jql endpoint (which replaced the removed GET /rest/api/3/search) uses cursor-based pagination and different request conventions. This commit fixes all three breaking changes: - expand is now a comma-delimited string, not a JSON array - pagination uses nextPageToken/isLast instead of startAt/total - fields explicitly requests ["*all"] (default is IDs only) Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
GetEntityProperty returns forge.ErrNotFound (404) when a property doesn't exist yet, which is the normal state on first poll. readLock and readLastCheck were treating this as an error, causing all issues to be skipped as "locked". Now they return nil/zero for 404, correctly treating missing properties as "unlocked" / "first poll". Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Jira timestamps have millisecond precision (e.g. 19:23:30.556) but lastCheck was stored and parsed with time.RFC3339 which truncates to whole seconds. This caused events at X.556s to pass the createdAt.After(lastCheck at X.000s) check on every subsequent poll. Switch to RFC3339Nano for both storage and parsing, with a fallback to RFC3339 for values written before this change. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- Update mock GetEntityProperty to return forge.ErrNotFound for missing properties, matching real Jira API behavior - TestReadLock_NotFound: verifies readLock returns nil (unlocked) when property doesn't exist - TestReadLastCheck_NotFound: verifies readLastCheck returns zero time when property doesn't exist - TestLastCheck_SubSecondPrecision: verifies RFC3339Nano round-trip preserves milliseconds and prevents re-dispatch - TestRunFirstPoll_NoLockProperty: full poll cycle with no prior entity properties (first-poll scenario) - Fix setLastCheck helper to use RFC3339Nano Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add a mock Jira REST API server (httptest-based, stateful) and Gherkin step definitions for testing the Jira poll input driver's dispatch path. - jiramock: stateful httptest server implementing search, comments, changelog, entity property CRUD, myself, and project role endpoints - jirapoll steps: Given/When/Then steps for mock setup, issue creation, comment and label manipulation, poller execution, and dispatch assertions - World extensions: JiraMockServer, JiraMockState, JiraConfigDir fields for per-scenario isolation with cleanup - Feature file: two scenarios exercising slash-command and label-change dispatch paths against the mock server The tests run the real poller logic (jira.Client → Poller → Router → dispatch records) against the mock server, validating the full integration without external dependencies. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Covers prerequisites, credential setup (Basic auth primary, OAuth 2.0 as untested alternative), scheduled workflow with dispatch step, poll coordination, and troubleshooting. The example workflow polls Jira on a cron, writes dispatch records, then triggers agent workflows via gh workflow run. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:52 PM UTC · Completed 9:11 PM UTC |
Site previewPreview: https://7db663d7-site.fullsend-ai.workers.dev Commit: |
PR Summary by QodoAdd Jira poll input driver with Jira REST client, dispatch output, and tests
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR adds substantial Go code (Jira REST client, poll driver, CLI wiring, tests) and the go label is consistently applied to Go PRs per repo conventions. Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Labels: PR adds behaviour test infrastructure (mock Jira server, step definitions, feature file) Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Low
Previous run (11)ReviewFindingsMedium
Low
Labels: PR adds new user guide (docs/guides/user/jira-integration.md) and behaviour test feature file Previous run (12)ReviewFindingsLow
Previous run (13)ReviewFindingsLow
Previous run (14)ReviewFindingsMedium
Low
Previous run (15)ReviewFindingsHigh
Medium
Low
Next steps:
|
|
🤖 Finished Fix · ❌ Failure · Started 9:12 PM UTC · Completed 9:26 PM UTC |
- releaseLock: verify lock ID before deleting to prevent TOCTOU race where a slow poller could delete a faster poller's lock - poller: reset dispatches slice at start of Run() to prevent accumulation across multiple calls - discover: mapStatusTransition uses destination status name from changelog instead of current issue statusCategory, fixing incorrect transition mapping when multiple status changes occur between polls - poller: don't advance maxTime past events that fail routing, so they can be retried next cycle - client: WithHTTPClient propagates to oauth2 token source - client: release mutex before HTTP call in oauth2 token refresh to avoid serializing all API calls under slow token endpoints - client: cap SearchIssues pagination at 200 pages (10k issues) - feature: remove @requires:jira-mock tag since mock is implemented Signed-off-by: Ralph Bean <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Review · ❌ Terminated · Started 9:19 PM UTC · Ended 9:37 PM UTC |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/.fullsend/actions/runs/30582388769 Details: |
…base URL Security review findings on the Jira client: - CheckRedirect stripped Authorization relative to the previous hop, but Go's client re-copies the initial request's headers onto every redirect hop and only strips them when leaving the initial domain-or-subdomain (host only, ignoring scheme). A same-host https->http downgrade chain or a subdomain chain therefore re-attached the Basic email:token credential on a later hop. This client only talks to one Jira Cloud origin, which never legitimately redirects its REST API off-origin, so it now refuses any redirect leaving the original scheme+host outright — strictly stronger than stripping, and it also removes the SSRF surface of following redirects to arbitrary internal hosts. Added same-origin, refused-off-origin, and 3-hop re-attach regression tests. - validateBaseURL now rejects a base URL with embedded credentials (https://user:token@host), which would otherwise be stored verbatim, propagated into every dispatched event's browse URL, and echoed into error text; error messages use url.Redacted() so a password can't leak to CI logs. - Added updateAuthor to the Comment type (consumed by the poller fix). Assisted-by: Claude (fix), Claude (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…its to the editor Jira entity properties (lock, lastCheck) are writable by anyone with Jira's Edit-Issues permission — broader than the role this driver maps to write — so they must be treated as untrusted input: - readLastCheck clamps the stored checkpoint: a future value is treated as unset (it would otherwise silently suppress all detection on the issue), and a value rewound before the backfill window is floored at the window's start. Without this, a rewind made firstPoll false, bypassing the backfill gate and replaying the issue's entire comment history — re-dispatching old privileged slash commands under their authors' roles, an ADR 0054 gate bypass. - isLockStale treats a future-dated lock as stale/reclaimable; previously time.Since on a future timestamp was negative and never exceeded the threshold, so one forged lock property wedged an issue permanently. - Edit-detected comments are attributed to updateAuthor (the editor), not the original author: a user with Edit-All-Comments could otherwise rewrite a privileged user's comment to inject a slash command that ran under the author's role. - Per-issue dispatch is capped (maxEventsPerIssue) so a flood — from a rewind or a bulk change — can't fan out unbounded agent workflow runs. - Entity-property lock IDs are logged quoted (log-injection into the CI audit trail); the property-key builder escapes dots so two dotted target repos (a.b/c vs a/b.c) can't collapse to the same lock/checkpoint namespace; a null/unexpected comment body extracts to "" not "<nil>". Assisted-by: Claude (fix), Claude (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Add a security note that Jira Edit-Issues permission confers write access to the poller's lock/lastCheck coordination properties, explain the clamp bounds, note that edited comments are attributed to the editor, and record the per-issue comment byte-budget as a tracked follow-up. Assisted-by: Claude (fix), Claude (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 11:15 PM UTC · Completed 11:35 PM UTC |
waynesun09
left a comment
There was a problem hiding this comment.
All prior review findings (122 threads across multiple automated sweeps and manual review) are resolved as of the latest commits. CI is green across all checks. LGTM.
|
🤖 Finished Retro · ✅ Success · Started 11:54 PM UTC · Completed 12:10 AM UTC |
Retro: PR #5778 — Jira poll input driverThis retro analyzed the full workflow for PR #5778, a large human-authored PR (+8,238 lines, 52 commits, 26 files) by Review quality: significant gap between agent and humanThe review agent produced ~16 distinct findings across 7 review cycles (0 HIGH, 2 MEDIUM, 14 LOW). The human reviewer found ~43 findings (11 HIGH, 32 MEDIUM). The agent missed every HIGH-severity issue on this PR, including cross-project privilege escalation, TOCTOU lock bypass, non-functional OAuth2 auth (later removed entirely), unbounded pagination, and credential leak in HTTP redirect handling. Three specific failure modes stood out:
The agent's severity calibration was systematically low: issues the human demonstrated were HIGH (security vulnerabilities, non-functional features, spec violations) were either missed entirely or rated LOW by the agent. Evidence for fullsend#5250 and agents#412. The agent did contribute some valid LOW-severity findings that led to code changes: naming convention ( Fix workflow: high failure rate is expected but noisy22 of 30 recent No new proposals filedAll improvement opportunities identified in this retro are covered by existing open issues. Rather than filing duplicates, the evidence from this PR is documented above for reference. The most impactful existing issues, ordered by expected workflow improvement, are:
|
Summary
internal/forge/jira/) with Basic and Bearer authinternal/jirapoll/) implementing ADR 0063's write-then-verify coordination protocolfullsend poll --input-driver jira-pollpkg/behaviourtest/drivers/jiramock/)docs/guides/user/jira-integration.md) with example workflow including dispatch stepThe poller converts Jira comments and label changes into NormalizedEvents and routes them through the existing HarnessRouter, so built-in agents (triage, code, review, etc.) work without modification. The example workflow includes a shell-based dispatch step that reads
dispatches.jsonand callsgh workflow runfor each record — a follow-up will wire this directly into the poller.Note: agent pre/post scripts don't understand Jira yet, so dispatched agents won't actually complete successfully. That's a follow-up.
Addresses #5758, #2264, #2265, #2268.
Related to #2269, #4885, #2513, #2266, #2267.
Test plan
go test ./internal/forge/jira/...— client unit tests (auth, pagination, error handling, entity properties)go test ./internal/jirapoll/...— poller unit tests (lock contention, stale cleanup, change detection, bot filtering, deduplication, role mapping)go test ./pkg/behaviourtest/...— behaviour test infrastructure (mock server, step definitions, slash command dispatch, label change dispatch)🤖 Generated with Claude Code