feat(jira): mature iteration feedback in place (#697)

[ayushtr-aws]

Matures one Jira iteration status comment from acknowledgement through heartbeat to terminal outcome. This is PR 3 of 3 for #580 and is based on #726.

Area

• cdk - infrastructure, handlers, constructs
• agent - Python runtime / Docker image
• cli - bgagent client
• docs - guides or design sources (docs/guides/, docs/design/)
• tooling - root mise.toml, scripts, CI workflows

Related

Closes #697

Parent: #580

Stack: #725 -> #726 -> #727

Changes

• Return and persist Jira comment IDs, and support in-place comment updates through Forge and OAuth paths.
• Generalize iteration heartbeats to Jira while guarding terminal claims so a running update cannot overwrite a final outcome.
• Assign standalone terminal maturation to fanout and orchestrated-child maturation to the reconciler before dependent restacking.
• Add shared terminal rendering, scoped registry and secret permissions, Forge proxy support, tests, and Jira setup documentation.

Verification

• mise run build
• Full CDK suite: 178 suites, 3,554 tests
• Focused CDK coverage: 429 tests
• Jira Forge proxy suite: 11 tests
• GITLEAKS_RANGE=origin/main..HEAD mise run security:secrets:range

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

[ayushtr-aws]

Submitting as a COMMENT review: GitHub blocks self-approval, but the substantive verdict is Approve with two non-blocking nits.

Verdict: Approve (with two non-blocking nits)

A well-structured, well-tested increment that matures a single Jira iteration comment from acknowledgement → heartbeat → terminal outcome, closing the last gap that made Jira feel second-class vs. Linear. Reviewed against the stack base feat/578-jira-orchestration-extension (PR 3 of 3, #725→#726→#727); I scoped out anything originating in the parent PRs.

Vision alignment

Fits the north star cleanly. It keeps the fire-and-forget path intact (all writes are advisory/best-effort, never gate the task), improves reviewable outcomes (a live heartbeat replaces the observed 22-min black box, plus a deterministic terminal comment carrying cost/turns/duration even on agent crash), and respects bounded blast radius: MAX_EDITS_PER_SWEEP cap retained, IAM scoped to the bgagent-jira-oauth-* prefix. No tenet is traded away; no ADR required.

Blocking issues

None.

Non-blocking suggestions / nits

1. Iteration-without-reply-id can duplicate the terminal comment on redelivery — cdk/src/handlers/fanout-task-events.ts:1629. The marker read is now gated !isIteration, so an iteration is never short-circuited by jira_final_comment_event_id. The iterationReplyId path is idempotent via claimTerminalReply, but an iteration whose webhook ack failed (no reply id captured) falls through to the ordinary post path at :1754, which writes the marker yet never reads it for iterations. On a partial-batch redelivery that iteration posts a second final comment. Narrow (needs a prior ack failure AND a sibling-channel-driven record redelivery) and advisory-only, so non-blocking — but consider having the no-reply-id iteration fallthrough either claim the terminal reply or honor the marker.

2. let reply; is implicitly untyped — cdk/src/handlers/orchestration-reconciler.ts:1215. The Jira/Linear branches assign it from upsertComment/postComment/upsertThreadedReply, and the reply === null / reply !== null control flow leans on that. An explicit let reply: CommentRef | null | undefined would make the tri-state (undefined = surface can't mature, null = attempted-and-failed) self-documenting and guard against a future refactor collapsing the distinction the block below depends on.

Documentation

Updated and in sync. docs/guides/JIRA_SETUP_GUIDE.md was revised (flow diagram, terminal-owner split, idempotency/claim semantics, five signed Forge operations, the new platform doctor step) and the Starlight mirror docs/src/content/docs/using/Jira-setup-guide.md carries the identical edits — verified the changed sentences match, so CI's mutation check will pass. No ADR needed (ADR-015 already covers the Jira integration; this is an implementation refinement). The stale "Jira has no comment-edit API" claims were correctly retired everywhere.

Tests & CI

Strong, failure-path-oriented coverage — not just happy paths:

• Fan-out: standalone-iteration split (pointer edit + separate result), orchestrated-iteration deferral to the reconciler, lost-claim (ConditionalCheckFailed) no-double-edit, failed/transient update with claim release + batch-retry escalation.
• Reconciler: Jira orchestration iteration maturation, failed-update claim release, redelivery-matures-once.
• Heartbeat sweep: Jira adapter edit path + strongly-consistent terminal-claim guard preventing a late heartbeat from regressing a settled outcome.
• jira-feedback: PUT/OAuth + Forge update_comment + 401→refresh→retry + missing-id rejection; comment-id extraction from the create response body.
• Forge proxy: update_comment happy path + traversal/missing-field rejection (invalid_update_comment_request).
• CLI: checkJiraAppIdentity warn/pass/incomplete/unreadable matrix.
• pipeline.py: _should_post_start_comment suppresses only the Jira pr-iteration duplicate ack.

Bootstrap synth-coverage: not applicable. No new CloudFormation resource types are introduced — the change adds a scoped IAM statement (secretsmanager:GetSecretValue/PutSecretValue on bgagent-jira-oauth-*) and an env var to the existing IterationHeartbeat Lambda, mirroring the reconciler's existing Linear grant. JiraWorkspaceRegistryTableName is a pre-existing stack output (not new). No bootstrap/ update required.

CI: all four checks green (build agentcore, secrets/deps/workflow scan, dead-code advisory, PR-title).

Review agents run

Operating as a review subagent, I applied each pr-review-toolkit agent's lens directly over the diff and full-file context in the worktree (I cannot spawn nested agent processes here):

• code-reviewer — guidelines/style, routing, L2/IAM patterns: pass (nit 2).
• silent-failure-hunter — every new write is best-effort with explicit claim-release + retryable-escalation branches; the heartbeat terminal-claim check fails open (returns false) by design. No swallowed failure that should surface. One idempotency edge (nit 1).
• type-design-analyzer — JiraPostResult/JiraUpdateResult discriminated unions are sound; JiraFinishedPointerKind is a tight enum. Nit 2 is the only typing observation.
• comment-analyzer — comments were faithfully updated to match new behavior; no stale/misleading comments remain (the old "no comment-edit API" lines are all gone).
• pr-test-analyzer — coverage assessed above; failure paths are well exercised.
• security-review judgment — applied (IAM + Forge proxy + path-traversal); see Human heuristics/Appropriateness. Nothing omitted as out-of-scope.

No agent was skipped.

Human heuristics

• Proportionality — Pass. Complexity matches the problem: the surface-neutral heartbeat generalization and the shared jira-status-comment.ts renderer avoid Linear/Jira copy-paste; the terminal-owner split (fan-out for standalone, reconciler for orchestrated child) is justified by the restack-ordering constraint.
• Coherence — Pass. Reuses the existing claimTerminalReply/releaseReplyClaim primitive and the Channel/channelForSource abstraction rather than inventing a parallel mechanism; renderJiraFinalStatusComment was relocated to the shared module with re-export, not duplicated.
• Clarity — Pass, modulo nit 2. Names communicate intent (renderJiraFinishedPointer, terminalReplyAlreadyClaimed, pointerKind); error branches log with structured event: keys.
• Appropriateness — Pass. Forge update_comment is validated against real Jira REST v3 semantics (PUT to /comment/{id}, ADF body object, numeric comment-id regex, issue-key traversal guard) and tested at the proxy boundary, not only via self-written mocks. IAM PutSecretValue is genuinely required (token refresh rewrites the cached OAuth bundle) and scoped to the tenant secret prefix — least-privilege preserved and consistent with the reconciler.

[nizar-lahlali]

Principal-architect review — PR #727 (Jira iteration feedback maturation)

Verdict: Request changes. This is a careful, well-tested change that cleanly extends the ack→heartbeat→terminal maturation model to Jira: contracts are mirrored (JiraPostResult now carries commentId, call sites updated across cdk/cli), the single-writer ack_replied_at claim is used consistently by fan-out, reconciler, and the heartbeat guard, the Forge update_comment op is properly allowlisted and traversal-guarded, and UA/least-privilege rules are honored. Two issues hold it back: an orchestrated-child terminal comment that renders degraded on the happy path, and a best-effort failure edge that leaves a status comment permanently promising a result that never posts. Neither is a security or contract-drift problem.

Vision alignment

Fits the "reviewable outcomes / fire-and-forget with maturing feedback" tenet — a single ABCA-owned comment matures in place rather than spamming the issue, terminal ownership is split so orchestrated restacking stays ordered (reconciler) while standalone settles on fan-out. Bounded blast radius preserved: Jira feedback stays strictly best-effort and never gates the coding task. No tenet trade-offs requiring an ADR.

Blocking

1. Orchestrated-child terminal comment renders degraded (literal markdown + no line breaks) — cdk/src/handlers/orchestration-reconciler.ts (Jira branch, ~L683–701).
The reconciler posts the terminal metrics comment via channel.postComment(target, renderJiraFinalStatusText({...})). renderJiraFinalStatusText (jira-status-comment.ts:1192) emits a \n-joined multi-line body and renders the PR link as markdown [url](url). But postComment → postIssueComment → toAdfDocument (jira-feedback.ts:61) wraps the entire string in a single ADF paragraph/text node — and per that file's own comments, ADF text nodes don't honor \n and a bare/markdown link is not linkified. Result: on every orchestrated-child iteration terminal comment (the exact path this PR adds), the header/metrics/PR/task-id lines collapse onto one line and the PR link shows as literal [https://…](https://…). The standalone fan-out path does this correctly via postIssueCommentAdf(buildAdfDocument(paragraphs)) — clickable link, separate paragraphs. The two terminal owners produce visibly different quality for the same outcome. The unit tests only assert the flattened string, so they don't catch it.
Fix: give the reconciler Jira path an ADF-capable route, e.g. postIssueCommentAdf(buildAdfDocument(renderJiraFinalStatusComment({...}))) as fan-out does, or add an ADF-body capability to the Jira channel. Verify against a real Jira comment before merge.

2. Terminal-result post failure leaves the pointer permanently promising a result that never lands — cdk/src/handlers/fanout-task-events.ts (standalone iteration branch, ~L323–357).
After updateIssueCommentAdf succeeds, the pointer reads ✅ Finished — result posted below. Then postIssueCommentAdf posts the metrics/result. If that result post fails terminally (retryable === false, or retryable && release === 'exhausted'), the code releases the claim and returns — leaving the pointer comment asserting "result posted below" with no result comment on the issue. The only trace is a WARN. The reconciler Jira branch (~L668–713 feeding the shared reply === null block) has the same shape.
Fix: on give-up, fold the full status into the pointer comment itself (re-updateIssueCommentAdf with buildAdfDocument(paragraphs)) so the single surviving comment tells the whole story, then return. Consider not releasing the claim on the terminal give-up path (mirror the reconciler's exhausted semantics) so nothing can later regress the comment.

Non-blocking suggestions / nits

• Malformed-success reported as failure — jira-feedback.ts createdCommentResult (~L961–977). When resp.ok was true but the body doesn't parse or lacks id, this returns {ok:false, retryable:false} — a comment that did land is reported as a failure, which can drive a duplicate re-post on retry. Prefer returning {ok:true, commentId:''} (comment exists, edit-in-place just isn't possible) and log the actual parse error + cloud_id/issue_key/task_id context rather than a bare generic warn.
• Ack post/persist split — jira-webhook-processor.ts (~L543–563). The single try/catch spans both the Jira postComment and the DDB iteration_reply_comment_id Update. If the comment posts but the Update fails, the id is never stored, so heartbeat/terminal maturation can't find it and will post a fresh comment instead of editing. Behavior can stay best-effort — just log the two outcomes distinctly so "posted-but-unlinked" is diagnosable.
• Duplicate claim helper — iteration-heartbeat-sweep.ts terminalReplyAlreadyClaimed (~L417–434). Re-implements the fail-open semantics of the existing terminalReplyClaimed in iteration-reply-claim.ts. Prefer reusing the shared helper so the fail-open contract lives in one place. (Fail-open here is the correct choice.)
• Stale doc comment — orchestration-channel-jira.ts header (~L25–33). Still says the Jira adapter "omits" edit-in-place / has no update helper, which now contradicts the real updateIssueComment in upsertComment.
• jira-feedback.ts LOW defense-in-depth: commentId originates from stored channel_metadata (untrusted-at-rest); injection is fully prevented downstream (proxy COMMENT_ID_RE, encodeURIComponent), but an early /^\d+$/ guard in updateIssueCommentAdf would keep validation symmetric and fail fast.

Documentation

Good. JIRA_SETUP_GUIDE.md updated (flow diagram, ack→heartbeat→terminal narrative, five signed proxy ops, platform doctor step) and the Starlight mirror docs/src/content/docs/using/Jira-setup-guide.md regenerated in sync — no "Fail build on mutation" risk. Issue #697 is approved and the change matches its acceptance criteria.

Tests & CI

All CI green (build agentcore, secrets/deps, title, dead-code). Coverage is strong and behavior-focused: comment-id capture, ADF-vs-text payloads, OAuth refresh+retry, single-writer claim/release, redelivery idempotency, terminal guard, Forge create+update with traversal rejection, and Linear/ordinary-Jira non-regression are all asserted; the ownership split is pinned from both sides. Gaps worth adding (not blocking):

1. Fan-out: pointer succeeds, result post fails retryably → claim released + record in batchItemFailures.
2. Fan-out: retryable failure with release === 'exhausted' → does not rethrow (no infinite redelivery).
3. Reconciler failure test: assert the metrics postComment is not called when the pointer upsertComment returns null.
4. Forge-app success path returns a usable commentId end-to-end (the primary production auth path); plus the numeric-id coercion branch of createdCommentResult.
5. Jira-specific heartbeat failure isolation (current test isolates via the Linear upsertThreadedReply path only).

Bootstrap synth-coverage: not applicable. The new secretsmanager:PutSecretValue on bgagent-jira-oauth-* is added as an inline policy to an existing Lambda role (CFN action iam:PutRolePolicy, already covered), the per-tenant secrets are CLI-created not CFN-managed, and this exact grant already ships on the orchestrator/reconciler. No ADR-002 re-bootstrap trigger met.

Review agents run

• code-reviewer (guidelines/style/correctness) — ran; surfaced blocking Installation: docker image inspect fails #1.
• silent-failure-hunter (error handling) — ran; surfaced blocking Docs: specify that you can't use the agent with the canned repo #2 and the createdCommentResult/ack-split nits.
• pr-test-analyzer (coverage) — ran; produced the test-gap list above.
• security-reviewer (Forge proxy, IAM, URL construction, ADR-002) — ran; 0 critical/high, confirmed proxy validation + IAM least-privilege sound and no bootstrap change needed.
• type-design-analyzer — omitted: no substantive new domain types (JiraFinalStatusInput/JiraFinishedPointerKind are thin DTO/union aliases).
• comment-analyzer — omitted as a dedicated pass; comment accuracy folded into code-reviewer (the one stale comment is noted above).

Human heuristics

• Proportionality — pass. Complexity matches the problem; the surface-neutral heartbeat generalization and the pointer+result two-comment terminal shape are justified by Jira's ADF/no-\n constraints.
• Coherence — concern: two terminal owners (fan-out vs reconciler) render the same outcome through different code paths with different fidelity (blocking Installation: docker image inspect fails #1). Same concept should produce the same comment.
• Clarity — concern: blocking Docs: specify that you can't use the agent with the canned repo #2 — a "Finished — result posted below" pointer that can outlive a failed result post surfaces a plausible-but-false state instead of the real outcome.
• Appropriateness — pass. Maintainable, mirrors existing Linear/Jira patterns, tests assert intended contract (_should_post_start_comment truth table is exemplary), integration validated against real Forge/Jira request shapes, not just self-written mocks.

[ayushtr-aws]

Addressed the new principal-architect review in 77209a43. All findings are owned by PR #727; PRs #725 and #726 do not need changes.

Blocking fixes:

• Orchestrated Jira iteration outcomes now use the same structured ADF renderer as standalone outcomes, preserving paragraphs, emphasis, and clickable PR links.
• If the separate result comment cannot be posted terminally, or transient retries are exhausted, the full outcome is folded into the existing status comment so it never permanently promises a missing result. The terminal claim remains held on give-up.

Suggestions addressed:

• Successful comment creates with malformed/missing IDs are treated as successful but non-editable, with contextual logging; numeric IDs and Forge app IDs are covered end to end.
• Acknowledgement posting and comment-ID persistence now have distinct diagnostics.
• The heartbeat sweep reuses the shared strongly-consistent terminal-claim helper.
• The Jira channel header is current.
• Comment updates reject non-numeric IDs before credential resolution.
• Added the requested retryable-result, exhausted-retry, no-post-after-pointer-failure, Forge-ID, numeric-ID, and Jira heartbeat-isolation coverage.

Validation: CDK lint/compile pass; targeted suites pass; full CDK suite passes (186 suites, 3,893 tests). Secrets scan is clean. The masking scan still reports the unchanged repository-wide 78-finding baseline.

[scottschreckengaust]

Verdict: Approve

Matures a single Jira iteration comment from acknowledgement → heartbeat → terminal outcome, closing the last gap that made Jira feel second-class vs. Linear. Reviewed as PR 3 of 3 against the stack base feat/578-jira-orchestration-extension (#725→#726→#727); anything originating in the parent PRs was scoped out. Both nits from the author's self-review (COMMENT, since GitHub blocks self-approval) are verified resolved at head 77209a4, all four CI checks are green, and I found no new blocking issue.

Vision alignment

Fits the north star cleanly. Fire-and-forget is preserved — every Jira write is advisory/best-effort and never gates or retries the coding task (webhook ack failure is caught and logged; heartbeat/terminal edits fail open). Reviewable outcomes improve: a live heartbeat replaces the observed 22-minute black box, and a deterministic terminal comment carries cost/turns/duration even when the agent crashes. Bounded blast radius holds: MAX_EDITS_PER_SWEEP cap retained, IAM scoped to the bgagent-jira-oauth-* secret prefix. No tenet is traded away; ADR-015 already covers the Jira integration and this is an implementation refinement, so no new ADR is required.

Blocking issues

None.

Non-blocking suggestions / nits

The two nits from the prior self-review are both verified fixed in the current head and require no further action:

1. Iteration-without-reply-id terminal-comment duplication (cdk/src/handlers/fanout-task-events.ts) — fixed in e279de52. The marker gate is now if ((!isIteration || !iterationReplyId) && task.jira_final_comment_event_id) (line 1630), so a no-ack-id iteration correctly falls through to the post-once marker path and a partial-batch redelivery no longer double-posts. Confirmed against current code.
2. let reply; implicitly untyped (cdk/src/handlers/orchestration-reconciler.ts) — fixed in e279de52. Now let reply: CommentRef | null | undefined; (line 1220), which makes the load-bearing tri-state (undefined = surface can't mature, null = attempted-and-failed) self-documenting. Confirmed.

One minor observation, purely optional: renderJiraFinishedPointerText / renderJiraFinalStatusText in the new jira-status-comment.ts exist as plain-text mirrors of the ADF renderers but appear to be exercised only by tests in this PR (the live paths use the ADF forms). If no production caller adopts them shortly, consider dropping them to avoid dead surface. Not blocking.

Documentation

Updated and in sync. docs/guides/JIRA_SETUP_GUIDE.md was revised (flow diagram, ordinary-vs-iteration terminal-owner split, claim/idempotency semantics, the fifth signed Forge operation update_comment, and a new platform doctor step) and the Starlight mirror docs/src/content/docs/using/Jira-setup-guide.md carries the identical edits — verified line-for-line, so CI's mutation check will pass. The stale "Jira has no comment-edit API" claims were correctly retired in the guide, the handler comments, and the TaskRecord.jira_final_comment_event_id doc-comment.

Tests & CI

Strong, failure-path-oriented coverage across all touched surfaces:

• Fan-out: standalone-iteration pointer-edit + separate result, orchestrated-iteration deferral to the reconciler, lost-claim (ConditionalCheckFailed) no-double-edit, failed/transient update with claim release + batch-retry escalation, exhausted-retry fold-into-status-comment, and the no-reply-id marker-honoring redelivery regression test.
• Reconciler: Jira orchestration-iteration maturation with metrics, failed-update claim release, terminal-post-failure fold, redelivery-matures-once.
• Heartbeat sweep: Jira adapter edit path + strongly-consistent terminal-claim guard preventing a late heartbeat from regressing a settled outcome; per-task edit-failure isolation.
• jira-feedback: PUT/OAuth path, Forge update_comment, 401→forced-refresh→retry, missing-comment (404) rejection, non-numeric comment-id rejection before credential resolution, and comment-id extraction (string, numeric-coerce, missing-id, non-JSON).
• Forge proxy: update_comment happy path plus traversal / missing-field rejection (invalid_update_comment_request).
• CLI: checkJiraAppIdentity warn/pass/incomplete/unreadable matrix.
• pipeline.py: _should_post_start_comment suppresses only the Jira pr-iteration duplicate ack.

Bootstrap synth-coverage: not applicable. No new CloudFormation resource types are introduced — the change adds a scoped IAM statement (secretsmanager:GetSecretValue/PutSecretValue on bgagent-jira-oauth-*) and an env var to the existing IterationHeartbeat Lambda, mirroring the reconciler's existing Linear grant. JiraWorkspaceRegistryTableName is a pre-existing stack output. No cdk/src/bootstrap/ update required.

Test performance: no re-enabling of aws:cdk:bundling-stacks; the new agent.test.ts case reuses the shared template. Clean.

CI: all four checks green (build agentcore, secrets/deps/workflow scan, dead-code advisory, PR-title).

Review agents run

Operating as a review subagent, I applied each pr-review-toolkit agent's lens directly over the diff and full-file worktree context (nested agent-process spawning is not available in this harness, so the lenses were applied by hand against the exact changed lines):

• code-reviewer — guidelines/style, routing (agent/cdk/cli/docs all landed in the correct package per AGENTS.md), L2/IAM patterns (formatArn + ArnFormat, no hardcoded ARNs): pass.
• silent-failure-hunter — every new write is best-effort with explicit claim-release and retryable-escalation branches; terminalReplyClaimed fails open (returns false) by design; the webhook ack post and its id-persistence are both wrapped and logged. No failure that should surface is swallowed.
• type-design-analyzer — JiraPostResult / JiraUpdateResult discriminated unions are sound; splitting the create result (commentId) from the update result (no id) is the right shape. JiraFinishedPointerKind is a tight enum. The reconciler reply tri-state is now explicitly typed.
• comment-analyzer — comments were faithfully updated to match new behavior; no stale/misleading comments remain (all "no comment-edit API" lines are gone from handlers, constructs, and the shared type doc).
• pr-test-analyzer — coverage assessed above; failure paths are well exercised, not just happy paths.
• security-review (lens) — applied to the IAM grant, the Forge proxy update_comment op, and the comment-id path-traversal guard; see Human heuristics / Appropriateness. PutSecretValue is genuinely required (OAuth refresh rewrites the cached bundle) and scoped to the tenant secret prefix. Nothing omitted as out-of-scope.

No in-scope agent was skipped.

Human heuristics

• Proportionality — Pass. The surface-neutral heartbeat generalization and the shared jira-status-comment.ts renderer avoid Linear/Jira copy-paste; the terminal-owner split (fan-out for standalone, reconciler for orchestrated child) is justified by the restack-ordering constraint, not gratuitous.
• Coherence — Pass. Reuses the existing claimTerminalReply/releaseReplyClaim primitive and the Channel/channelForSource abstraction rather than inventing a parallel mechanism; renderJiraFinalStatusComment was relocated to the shared module and re-exported, not duplicated.
• Clarity — Pass. Names communicate intent (renderJiraFinishedPointer, terminalReplyClaimed, pointerKind, credentialsRef); error branches log with structured event: keys. Minor: the plain-text renderer mirrors are currently test-only (see nits).
• Appropriateness — Pass. Forge update_comment is validated against real Jira REST v3 semantics (PUT to /comment/{id}, ADF body object, numeric comment-id regex, issue-key traversal guard) and tested at the proxy boundary, not only via self-written mocks. The write helper's commentId: '' sentinel on a create-succeeded-but-unparseable-id response correctly prefers not to duplicate — a sound fail-safe choice.