Merge branch 'main' into github-actions/upgrade-main

Author: scottschreckengaust

agent/src/jira_reactions.py

@@ -1,9 +1,15 @@
 """Jira issue-comment helper for Jira-origin tasks.
 
-Posts a "starting" comment on the originating Jira issue at task start and a
-terminal "succeeded / failed (+ PR link)" comment at the end — the Jira
-analogue of ``linear_reactions`` (Linear uses emoji reactions; Jira's REST
-API has no lightweight reaction primitive, so comments are the right tool).
+Posts a "starting" comment on the originating Jira issue at task start — the
+Jira analogue of ``linear_reactions`` (Linear uses emoji reactions; Jira's
+REST API has no lightweight reaction primitive, so comments are the right
+tool).
+
+The *terminal* status comment is NOT posted from here: since issue #573 the
+deterministic fan-out plane (``cdk/src/handlers/fanout-task-events.ts``
+``dispatchToJira``) owns it, so it carries cost/turns/duration and fires even
+when this agent crashes before completing. This module only owns the start
+comment.
 
 Why a direct REST call instead of MCP: Atlassian's Remote MCP
 (``mcp.atlassian.com``) requires an interactive, browser-based OAuth 2.1
@@ -182,32 +188,13 @@ def comment_task_started(
     log("TASK", f"jira_reactions: comment_task_started issue={issue_key} ok={ok}")
 
 
-def comment_task_finished(
-    channel_source: str,
-    channel_metadata: dict[str, str] | None,
-    success: bool,
-    pr_url: str | None = None,
-) -> None:
-    """Post a terminal status comment on the Jira issue. No-op for non-Jira tasks."""
-    target = _enabled(channel_source, channel_metadata)
-    if not target:
-        return
-    cloud_id, issue_key = target
-    if success:
-        text = "✅ ABCA finished this task."
-        if pr_url:
-            text += f" Pull request: {pr_url}"
-        else:
-            text += " No pull request was opened."
-    else:
-        text = "❌ ABCA could not complete this task. Check the agent logs for details."
-        if pr_url:
-            text += f" A pull request was opened anyway: {pr_url}"
-    ok = _post_comment(cloud_id, issue_key, text)
-    log(
-        "TASK",
-        f"jira_reactions: comment_task_finished issue={issue_key} success={success} ok={ok}",
-    )
+# NOTE: there is deliberately no ``comment_task_finished`` here. Since issue
+# #573 the deterministic fan-out plane
+# (``cdk/src/handlers/fanout-task-events.ts`` ``dispatchToJira``) owns the Jira
+# terminal comment — it carries cost/turns/duration and, crucially, fires even
+# when the agent crashes before completing (max-turns, OOM). The agent only
+# posts the *start* comment (``comment_task_started`` above); posting a terminal
+# comment here too would double-comment on the issue.
 
 
 def _reset_state_for_testing() -> None:

agent/src/pipeline.py

@@ -23,7 +23,7 @@
     resolve_linear_api_token,
 )
 from context import assemble_prompt, fetch_github_issue
-from jira_reactions import comment_task_finished, comment_task_started
+from jira_reactions import comment_task_started
 from linear_reactions import react_task_finished, react_task_started
 from models import AgentResult, HydratedContext, RepoSetup, TaskConfig, TaskResult
 from observability import current_otel_trace_id, task_span
@@ -1106,14 +1106,14 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
                 started_reaction_id=linear_eyes_reaction_id,
             )
 
-            # Terminal status comment on the Jira issue (REST shim, with the
-            # PR link when one was opened). No-op for non-Jira tasks.
-            comment_task_finished(
-                config.channel_source,
-                config.channel_metadata,
-                success=(overall_status == "success"),
-                pr_url=pr_url,
-            )
+            # NOTE: the terminal status comment on the Jira issue is NOT posted
+            # here. Since issue #573 the deterministic fan-out plane
+            # (``cdk/src/handlers/fanout-task-events.ts`` ``dispatchToJira``)
+            # owns the Jira final-status comment — it carries cost/turns/
+            # duration and, crucially, fires even if this agent crashes before
+            # reaching this point (max-turns, OOM). Posting here too would
+            # double-comment. The agent still posts the *start* comment
+            # (``comment_task_started`` above) for in-flight progress.
 
             # --trace trajectory S3 upload (design §10.1). Runs AFTER
             # post-hooks but BEFORE ``write_terminal`` so the resulting
@@ -1244,14 +1244,11 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
                 success=False,
                 started_reaction_id=linear_eyes_reaction_id,
             )
-            # Best-effort failure comment on the Jira issue. No-op for
-            # non-Jira tasks; network failures are swallowed.
-            comment_task_finished(
-                config.channel_source,
-                config.channel_metadata,
-                success=False,
-                pr_url=None,
-            )
+            # NOTE: no Jira failure comment here — the fan-out plane's
+            # ``dispatchToJira`` (issue #573) owns the Jira terminal comment
+            # and fires on the platform side even when this crash path runs,
+            # so posting here would double-comment. (Contrast the Linear ❌
+            # reaction above, which the fan-out plane does not replicate.)
             raise
 
 

agent/tests/test_jira_reactions.py

@@ -7,7 +7,7 @@
 import pytest
 
 import jira_reactions
-from jira_reactions import comment_task_finished, comment_task_started
+from jira_reactions import comment_task_started
 
 JIRA_META = {"jira_cloud_id": "cloud-1", "jira_issue_key": "KAN-1"}
 
@@ -36,7 +36,6 @@ def test_non_jira_source_is_noop(self, monkeypatch):
         monkeypatch.setenv("JIRA_API_TOKEN", "jira_at")
         with patch("jira_reactions.requests.post") as post:
             comment_task_started("linear", JIRA_META)
-            comment_task_finished("linear", JIRA_META, success=True)
             post.assert_not_called()
 
     def test_empty_metadata_is_noop(self, monkeypatch):
@@ -74,31 +73,14 @@ def test_skips_when_token_missing(self, monkeypatch):
             post.assert_not_called()
 
 
-class TestFinishComment:
-    def test_success_with_pr_includes_pr_url(self, monkeypatch):
-        monkeypatch.setenv("JIRA_API_TOKEN", "jira_at")
-        with patch("jira_reactions.requests.post", return_value=_resp(201)) as post:
-            comment_task_finished(
-                "jira", JIRA_META, success=True, pr_url="https://github.com/o/r/pull/7"
-            )
-        text = post.call_args[1]["json"]["body"]["content"][0]["content"][0]["text"]
-        assert "✅" in text
-        assert "https://github.com/o/r/pull/7" in text
-
-    def test_success_without_pr_notes_no_pr(self, monkeypatch):
-        monkeypatch.setenv("JIRA_API_TOKEN", "jira_at")
-        with patch("jira_reactions.requests.post", return_value=_resp(201)) as post:
-            comment_task_finished("jira", JIRA_META, success=True, pr_url=None)
-        text = post.call_args[1]["json"]["body"]["content"][0]["content"][0]["text"]
-        assert "✅" in text
-        assert "No pull request" in text
+class TestTerminalCommentDemoted:
+    """Since issue #573 the fan-out plane (``dispatchToJira``) owns the Jira
+    terminal comment, so the agent no longer exposes ``comment_task_finished``.
+    This pins the demotion so a future refactor can't silently re-introduce a
+    duplicate terminal comment on the agent side."""
 
-    def test_failure_comment(self, monkeypatch):
-        monkeypatch.setenv("JIRA_API_TOKEN", "jira_at")
-        with patch("jira_reactions.requests.post", return_value=_resp(201)) as post:
-            comment_task_finished("jira", JIRA_META, success=False, pr_url=None)
-        text = post.call_args[1]["json"]["body"]["content"][0]["content"][0]["text"]
-        assert "❌" in text
+    def test_comment_task_finished_is_gone(self):
+        assert not hasattr(jira_reactions, "comment_task_finished")
 
 
 class TestFailureIsSwallowed:
@@ -107,7 +89,6 @@ def test_http_500_does_not_raise(self, monkeypatch):
         with patch("jira_reactions.requests.post", return_value=_resp(500, "boom")):
             # Must not raise.
             comment_task_started("jira", JIRA_META)
-            comment_task_finished("jira", JIRA_META, success=True)
 
     def test_request_exception_does_not_raise(self, monkeypatch):
         import requests
@@ -130,7 +111,7 @@ def test_opens_after_threshold_consecutive_401s(self, monkeypatch):
             calls_after_open = post.call_count
             # Further calls short-circuit without hitting the network.
             comment_task_started("jira", JIRA_META)
-            comment_task_finished("jira", JIRA_META, success=True)
+            comment_task_started("jira", JIRA_META)
             assert post.call_count == calls_after_open
 
     def test_2xx_resets_failure_counter(self, monkeypatch):

cdk/bootstrap/bootstrap-template.yaml

@@ -930,6 +930,7 @@ Resources:
               - route53resolver:UpdateFirewallDomains
               - route53resolver:AssociateFirewallRuleGroup
               - route53resolver:DisassociateFirewallRuleGroup
+              - route53resolver:UpdateFirewallRuleGroupAssociation
               - route53resolver:GetFirewallRuleGroupAssociation
               - route53resolver:ListFirewallRuleGroupAssociations
               - route53resolver:UpdateFirewallConfig
@@ -1191,6 +1192,7 @@ Resources:
               - cloudwatch:TagResource
               - cloudwatch:UntagResource
               - logs:CreateDelivery
+              - logs:UpdateDeliveryConfiguration
               - logs:DescribeDeliveries
               - logs:GetDelivery
               - logs:GetDeliveryDestination

cdk/bootstrap/policies/infrastructure.json

@@ -153,6 +153,7 @@
         "route53resolver:UpdateFirewallDomains",
         "route53resolver:AssociateFirewallRuleGroup",
         "route53resolver:DisassociateFirewallRuleGroup",
+        "route53resolver:UpdateFirewallRuleGroupAssociation",
         "route53resolver:GetFirewallRuleGroupAssociation",
         "route53resolver:ListFirewallRuleGroupAssociations",
         "route53resolver:UpdateFirewallConfig",

cdk/bootstrap/policies/observability.json

@@ -44,6 +44,7 @@
         "cloudwatch:TagResource",
         "cloudwatch:UntagResource",
         "logs:CreateDelivery",
+        "logs:UpdateDeliveryConfiguration",
         "logs:DescribeDeliveries",
         "logs:GetDelivery",
         "logs:GetDeliveryDestination",

cdk/src/bootstrap/policies/infrastructure.ts

@@ -190,6 +190,7 @@ export function infrastructurePolicy(): iam.PolicyDocument {
           'route53resolver:UpdateFirewallDomains',
           'route53resolver:AssociateFirewallRuleGroup',
           'route53resolver:DisassociateFirewallRuleGroup',
+          'route53resolver:UpdateFirewallRuleGroupAssociation',
           'route53resolver:GetFirewallRuleGroupAssociation',
           'route53resolver:ListFirewallRuleGroupAssociations',
           'route53resolver:UpdateFirewallConfig',

cdk/src/bootstrap/policies/observability.ts

@@ -79,6 +79,7 @@ export function observabilityPolicy(): iam.PolicyDocument {
           'cloudwatch:TagResource',
           'cloudwatch:UntagResource',
           'logs:CreateDelivery',
+          'logs:UpdateDeliveryConfiguration',
           'logs:DescribeDeliveries',
           'logs:GetDelivery',
           'logs:GetDeliveryDestination',

cdk/src/constructs/fanout-consumer.ts

@@ -106,6 +106,27 @@ export interface FanOutConsumerProps {
    */
   readonly linearOauthSecretArnPattern?: string;
 
+  /**
+   * JiraWorkspaceRegistryTable — the Jira dispatcher reads this to resolve
+   * per-tenant OAuth tokens at comment-post time (issue #573). Optional:
+   * when omitted, the dispatcher logs and skips so a deployment without
+   * Jira onboarding doesn't accumulate dangling IAM grants. Mirrors
+   * ``linearWorkspaceRegistryTable``.
+   */
+  readonly jiraWorkspaceRegistryTable?: dynamodb.ITable;
+
+  /**
+   * Secrets Manager ARN-prefix pattern for per-tenant Jira OAuth bundles.
+   * Mirrors ``linearOauthSecretArnPattern`` — typically
+   * ``bgagent-jira-oauth-*``. Required when ``jiraWorkspaceRegistryTable``
+   * is set; without it the dispatcher would resolve the registry row but
+   * fail at the SM GetSecretValue call. GetSecretValue + PutSecretValue
+   * because the resolver rotates an expiring token in place (the fan-out
+   * Lambda is trusted stack code, same as the orchestrator + webhook
+   * processor).
+   */
+  readonly jiraOauthSecretArnPattern?: string;
+
   /**
    * Maximum batch size delivered to the Lambda per invocation.
    *
@@ -231,6 +252,29 @@ export class FanOutConsumer extends Construct {
       }));
     }
 
+    // Jira dispatcher plumbing (issue #573). Same guarded shape as Linear:
+    // a deployment without Jira onboarding gets no IAM grants and the
+    // dispatcher logs-and-skips on missing env. The registry table resolves
+    // the per-tenant OAuth-secret ARN; the secret holds the access token
+    // ``postIssueCommentAdf`` uses to POST the final-status comment via the
+    // Jira REST v3 API.
+    if (props.jiraWorkspaceRegistryTable) {
+      props.jiraWorkspaceRegistryTable.grantReadData(this.fn);
+      this.fn.addEnvironment(
+        'JIRA_WORKSPACE_REGISTRY_TABLE_NAME',
+        props.jiraWorkspaceRegistryTable.tableName,
+      );
+    }
+    if (props.jiraOauthSecretArnPattern) {
+      this.fn.addToRolePolicy(new iam.PolicyStatement({
+        // GetSecretValue + PutSecretValue: resolving an expiring token
+        // refreshes it in place — same grants the orchestrator + Jira
+        // webhook-processor Lambdas hold.
+        actions: ['secretsmanager:GetSecretValue', 'secretsmanager:PutSecretValue'],
+        resources: [props.jiraOauthSecretArnPattern],
+      }));
+    }
+
     // Alarm on any record landing in the DLQ. Notifications are
     // best-effort by design, so individual failures don't fail the
     // batch — which means the DLQ is the ONLY persistent signal of a
@@ -244,7 +288,7 @@ export class FanOutConsumer extends Construct {
       threshold: 1,
       evaluationPeriods: 1,
       alarmDescription:
-        'Fan-out DLQ has undelivered task-event records — Slack/GitHub/Linear notifications are failing',
+        'Fan-out DLQ has undelivered task-event records — Slack/GitHub/Linear/Jira notifications are failing',
       treatMissingData: cloudwatch.TreatMissingData.NOT_BREACHING,
     });