[Epic] Automatic CI Failure Follow-Up Controls

[propr-dev]

Automatic CI Failure Follow-Up Controls

This Epic PR aggregates all changes for the plan: Automatic CI Failure Follow-Up Controls

Issues in this Epic

• Add repo-level configuration for automatic CI failure follow-up #1926: Add repo-level configuration for automatic CI failure follow-up
• Post automatic follow-up comments when enabled repo CI fails #1927: Post automatic follow-up comments when enabled repo CI fails
• Expose automatic CI follow-up controls in CLI, UI, and docs #1928: Expose automatic CI follow-up controls in CLI, UI, and docs

Auto-close

When this PR is merged, the following issues will be automatically closed:

Fixes #1926
Fixes #1927
Fixes #1928

---

Created automatically by ProPR

[integry]

packages/core/src/webhook/ciFailureFollowup.ts
}

function escapeInlineCode(value: string): string {
return value.replace(//gu, '\\');
github-advanced-security commented 2 hours ago

github-advanced-security
bot
2h ago
High

CodeQL / Incomplete string escaping or encoding
This does not escape backslash characters in the input.

Show more details

Suggested changeset 1 (1)

packages/core/src/webhook/ciFailureFollowup.ts
Original file line number Diff line number Diff line change
@@ -333,7 +333,7 @@
333 333
}
334 334
335 335
function escapeInlineCode(value: string): string {
336 -
return value.replace(//gu, '\\');
336 +
return value.replace(/([\`])/gu, '\$1');
337 337
}
338 338
339 339
function escapeInlineMarkdown(value: string): string {

Commit suggestion

Add suggestion to batch




[propr-dev]

✅ Applied the requested follow-up changes in commit 765989b

Summary of Changes

Updated escapeInlineCode to escape both backslashes and backticks, resolving the CodeQL finding.

Validation:

• Core TypeScript type-check passed.
• Targeted CI follow-up tests passed: 4/4.
• git diff --check passed.
• No commit created, as requested.

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @integry
• Time: 1m 39s
• Tokens: 133,746 (132,428 in / 1,318 out)
• Cost: $0.21

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5395004090✓

[integry]

/review codex

[propr-dev]

✅ AI Code Review Complete requested by @integry

Posted 1 review:

• codex

View Task Details

[propr-dev]

🔍 AI Code Review — codex

Overall Evaluation

This PR adds an off-by-default repository setting across the API, CLI, UI, documentation, and webhook processing, then generates deduplicated CI-failure comments for the normal follow-up worker. It needs minor, targeted changes before merge: duplicate repository configurations are resolved inconsistently, and delayed CI events can launch work for obsolete commits.

✅ Safe configuration default — Missing values normalize to false, invalid values are rejected, and legacy clients preserve existing explicit settings.

✅ Strong deduplication and evidence handling — Redis claims prevent concurrent duplicates, comment-marker scans survive Redis expiry, and check-run annotations take priority over generic summaries.

✅ Broad surface coverage — CLI, UI, API, documentation, and focused tests were updated, with all 12 current required checks passing.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F1: 🔴 Duplicate repositories resolve unpredictably

• Required behavior: The repository-level flag must be persisted, displayed, and consumed deterministically for every supported repository configuration.
• Evidence: packages/core/src/daemon/configLoader.ts:46 — .find() uses only the first configuration matching owner/repo, while propr-ui/src/hooks/useRepositoryManagement.ts:335 toggles individual IDs, allowing duplicate branch entries for the same repository to hold conflicting values.
• Minimum fix: Enforce repository-level consistency across same-name entries—for example, toggle all matching entries together and aggregate enabled state deterministically—and add a regression test with duplicate branch configurations.

F2: 🔴 Stale CI can launch follow-up work

• Required behavior: Autonomous CI follow-up must act on a failure relevant to the PR’s current head, rather than obsolete CI results delivered after another commit was pushed.
• Evidence: packages/core/src/webhook/checkRunHandler.ts:258 and packages/core/src/webhook/checkRunHandler.ts:331 — Both failure paths post for associated PRs without verifying that evidence.sha still equals the PR’s live head SHA.
• Minimum fix: Fetch each PR’s current head immediately before posting, skip when it differs from the failing SHA, and cover stale check-run and legacy-status events in tests.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Use valid code-span escaping

packages/core/src/webhook/ciFailureFollowup.ts:335 prefixes backticks with backslashes, but Markdown code spans do not interpret those backslashes as escaping delimiters. Selecting a backtick delimiter longer than any run in the value would keep unusual CI names formatted correctly. This is optional hardening for uncommon check names.

S2: 🟢 Clarify ignore-keyword precedence

packages/core/src/webhook/commentEventHandler.ts exempts marked CI comments from configured follow-up ignore keywords. Document and directly test that policy so administrators know repository opt-in takes precedence; this is optional because that precedence can reasonably be intentional.

Score

The implementation is cohesive, well tested, and currently green, but two localized runtime correctness issues still make autonomous follow-up unsafe or inconsistent in supported scenarios.

Score: 6/10

---

🤖 Review Details

• Model: GPT-5.6 Sol
• Time: 6m 31s
• Tokens: 75,932 (63,852 in / 12,080 out)
• Cost: $0.65

View Task

---

💡 Next step: Comment /fix to address F# merge blockers only.
F# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. Suggestions require a separate ordinary follow-up request.

_{🤖 Review by ProPR}

[integry]

/ultrafix

[propr-dev]

🔄 Ultrafix loop started (goal: 8/10, max cycles: 10)

First action: /fix

💡 Tip: Remove the ultrafix label from this PR to stop further ultrafix cycles.

[propr-dev]

✅ Applied the requested follow-up changes in commit f4797a5

Addressed 1 AI review comment (IDs: 5396025800)

Summary of Changes

Implemented only F1 and F2.

• Duplicate branch configurations now resolve case-insensitively and order-independently; UI loads, toggles, and saves the repository-level flag consistently across matching entries.
• Failed check-run and legacy-status events now verify each PR’s live head SHA before posting, skipping stale failures.
• Added regression coverage for duplicate branches and both stale webhook types.

Verification passed:

• Backend tests: 81 passed
• UI tests: 14 passed
• Core and UI type checks
• Core and UI lint
• git diff --check

No commit was created.

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @integry
• Time: 6m 24s
• Tokens: 1,480,020 (1,466,150 in / 13,870 out)
• Cost: $1.59

Undo Changes • View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}
Processing comment ID: 5402655025✓

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• claude:claude-sonnet-5

View Task Details

[propr-dev]

🔍 AI Code Review — claude:claude-sonnet-5

Overall Evaluation

This is a large, well-organized PR implementing automatic CI-failure follow-up across backend validation, webhook handling, comment-pipeline trust boundaries, CLI, and UI, per issues #1926–#1928. The backend validation, dedup/claim logic in ciFailureFollowup.ts, and the marker-based bot-authorship trust boundary in commentEventHandler.ts are carefully built and well tested. However, the UI's "add repository" flow contains a genuine logic bug that can silently mutate the persisted autoFollowupOnFailedCi state of other, unrelated monitored-branch entries for the same repository — including silently disabling a setting an admin had explicitly turned on — which conflicts with both the safety intent of the feature and the OR-aggregation invariant this same PR establishes elsewhere. All current-head checks pass.

✅ Solid dedup/claim design — postCiFailureFollowup in packages/core/src/webhook/ciFailureFollowup.ts uses a Redis SET NX claim with TTL, releases the claim on failure to allow retries, and additionally scans existing PR comments for the hidden marker as a defense-in-depth check against redelivered webhooks, backed by dedicated tests in test/ciFailureFollowup.test.ts.

✅ Trust boundary correctly scoped — prepareCiFollowupComment in packages/core/src/webhook/commentEventHandler.ts requires both a configured bot username and the hidden marker before bypassing author filtering, and this is explicitly verified by the "does not let an unrelated bot use the CI marker to bypass author filtering" test in test/commentEventHandler.switch-use.test.ts.

✅ Defaults enforced consistently at the API boundary — normalizeRepoConfig in packages/api/routes/configRepoValidation.ts rejects non-boolean values and defaults missing values to false, and configRoutes.ts's getRepos/postRepos correctly coerce/preserve the field, with focused unit tests in packages/api/test/configRepoValidation.test.ts and configRepoRoutes.test.ts.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F3: 🔴 Adding a branch-duplicate repository entry can silently disable an already-enabled CI follow-up setting

• Required behavior: Issue Add repo-level configuration for automatic CI failure follow-up #1926 requires the flag to persist correctly and change only via explicit admin action ("Default the field to false... unless explicitly enabled by an admin"). This PR's own OR-aggregation invariant (isAutoCiFollowupEnabledForRepository in packages/core/src/daemon/configLoader.ts, and the identical OR logic in loadRepos in the same UI hook file) establishes that any one enabled branch entry should keep the repo's automation effectively enabled. handleAddRepo violates that invariant by overwriting sibling entries with the raw new value instead of OR-merging.
• Evidence: propr-ui/src/hooks/useRepositoryManagement.ts (handleAddRepo): const newRepos = [...repos.map(repo => getRepositoryConfigKey(repo.name) === repositoryKey ? { ...repo, autoFollowupOnFailedCi } : repo), newEntry]; — this unconditionally assigns the new form checkbox's raw boolean (default false) to every existing repo entry sharing the same name, rather than repo.autoFollowupOnFailedCi || autoFollowupOnFailedCi. Given the product explicitly supports "the same repository multiple times with different base branches" (per AddRepositoryModal.tsx's own help text), an admin who previously enabled follow-up on branch A and later adds monitoring for branch B via the modal — leaving the (default-unchecked) checkbox unset — will have branch A's persisted autoFollowupOnFailedCi silently flipped to false and saved via performAutoSave, without ever interacting with that control.
• Minimum fix: In handleAddRepo, OR-merge rather than overwrite when synchronizing same-named entries (e.g. autoFollowupOnFailedCi: repo.autoFollowupOnFailedCi || autoFollowupOnFailedCi), or simply leave existing entries untouched and rely on the backend's OR-based isAutoCiFollowupEnabledForRepository for effective enablement, matching the "adds ... without changing existing repositories" intent already stated in the adjacent test name.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 CLI repo toggle does not synchronize duplicate-name branch entries

updateRepo in packages/cli/src/api/repos.ts locates only the first repo entry matching a given name (findIndex) and updates just that one entry. Because isAutoCiFollowupEnabledForRepository treats the flag as OR'd across all same-named branch entries, running propr repo toggle owner/repo --no-auto-ci-followup on a repository that has multiple monitored branches (a supported, documented configuration) may appear to succeed while leaving the effective automation enabled via another untouched entry. This is optional to fix now because the CLI's own addRepo cannot itself create such duplicate-name entries (it throws "already being monitored"), so the gap only surfaces when branches were first added through the UI and later managed via CLI.

S2: 🟢 Markdown injection risk from CI failure text in follow-up comments

buildCiFailureFollowupComment and buildAnnotationExcerpt in packages/core/src/webhook/ciFailureFollowup.ts embed raw check-run annotation message/raw_details/title text (and status description) inside a blockquote without escaping content that could itself contain triple backticks, additional blank lines, or further HTML comments, potentially breaking the intended rendering or (in a contrived case) appending an attacker-influenced trailing marker if CI output is attacker-controlled (e.g., a test name printed from PR-controlled code). This is a hardening item rather than a blocker since the content still originates from the repository's own CI system, not directly from PR comments.

S3: 🟢 Duplicate-branch scenario for handleAddRepo lacks direct test coverage

The added test "adds the selected automatic CI setting without changing existing repositories" in propr-ui/src/hooks/useRepositoryManagement.test.tsx uses a distinct repository name (integry/new) for the new entry, so it never exercises the same-name/different-branch sync path described in F1. Adding a test that adds a second baseBranch entry for an already-configured repo name, with the checkbox left unchecked, would have caught the regression and should be added alongside the fix.

S4: 🟢 Sequential per-PR follow-up posting in webhook handlers

handleCheckRunEvent and handleStatusEvent in packages/core/src/webhook/checkRunHandler.ts await postCiFailureFollowup sequentially inside a for loop over associated PRs. This is fine functionally, but for check runs associated with several open PRs it serializes GitHub API calls; parallelizing with Promise.allSettled could reduce webhook processing latency. Purely a performance nit, not correctness-affecting.

Score

One localized but real correctness bug (F1) can silently disable (or otherwise desynchronize) an already-enabled, safety-relevant repo setting outside of any explicit admin toggle of that control; the rest of the PR (validation, dedup, trust boundary, CLI/API plumbing) is solid and well tested, and all current-head checks pass.

Score: 4/10

---

🤖 Review Details

• Model: Claude Sonnet 5
• Time: 4m 47s
• Tokens: 118,835 (89,754 in / 29,081 out)
• Cost: $0.52

View Task

---

💡 Next step: Comment /fix to address F# merge blockers only.
F# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. Suggestions require a separate ordinary follow-up request.

_{🤖 Review by ProPR}

[propr-dev]

✅ Applied the requested follow-up changes in commit 3539351

Addressed 1 AI review comment (IDs: 5402783498)

Summary of Changes

Implemented F3 only.

• handleAddRepo now OR-merges the CI follow-up flag, preserving an existing enabled setting when another branch is added unchecked.
• Added a regression test for that scenario.

Files changed:

• useRepositoryManagement.ts
• useRepositoryManagement.test.tsx

Verification passed:

• Hook tests: 15/15
• UI TypeScript typecheck
• git diff --check

No commit created.

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @propr-ultrafix
• Time: 2m 2s
• Tokens: 193,422 (190,940 in / 2,482 out)
• Cost: $0.29

View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• claude:claude-sonnet-5

View Task Details

[propr-dev]

🔍 AI Code Review — claude:claude-sonnet-5

Overall Evaluation

This PR implements the full three-part feature (repo-level config flag, automatic CI-failure follow-up comments, and CLI/UI/docs exposure) described in issues #1926–#1928. The implementation is broad but consistently defensive: the new field defaults to false end-to-end, the backend preserves omitted values on partial writes, and the CI follow-up path is gated behind an explicit enablement check before any GitHub/Redis I/O occurs. Test coverage is unusually thorough across API validation, CLI, the webhook handlers, and the UI hook. All current-head checks are green.

One correctness issue was found in the UI's handling of duplicate branch-specific repository entries that share a name: the loader's OR-aggregation is written back into the persisted local state instead of being kept as a display-only derivation, so an unrelated save (e.g. toggling enabled on a different field of the same/duplicate entry) can silently overwrite a deliberately-set false value to true for that entry. This is narrow in trigger conditions but is a genuine data-integrity regression in newly-added code and should be fixed before merge.

✅ Safe-by-default gating — postCiFailureFollowup checks isAutoCiFollowupEnabledForRepository first and returns before touching GitHub or Redis when disabled (packages/core/src/webhook/ciFailureFollowup.ts), verified by the "does not access GitHub or Redis when disabled" test.

✅ Layered deduplication — Redis NX claim plus a GitHub comment marker scan protects against both webhook redelivery races and process-restart/TTL-expiry duplicate posting (packages/core/src/webhook/ciFailureFollowup.ts), with claim release on failure so legitimate retries aren't blocked.

✅ Backward-compatible persistence semantics — postRepos in packages/api/routes/configRoutes.ts preserves the previously-stored autoFollowupOnFailedCi value when a client omits the field, rather than defaulting it to false and clobbering existing admin configuration, verified by configRepoRoutes.test.ts.

Merge blockers

Every finding below was introduced by this PR and must be resolved before merging.

F4: 🔴 UI silently overwrites a persisted per-entry autoFollowupOnFailedCi value on unrelated saves

• Required behavior: Issue Add repo-level configuration for automatic CI failure follow-up #1926 requires the field to be "Persist[ed], validat[ed], return[ed], and consume[d]... through the existing /api/config/repos flow" faithfully, and to remain off "unless explicitly enabled by an admin." An unrelated UI action must not silently change a field the admin never touched.
• Evidence: propr-ui/src/hooks/useRepositoryManagement.ts in loadRepos: autoCiFollowupByRepository is computed as an OR-aggregate across all entries sharing a repository name, and then written back into every one of those entries via validRepos = parsedRepos.map(repo => ({ ...repo, autoFollowupOnFailedCi: autoCiFollowupByRepository.get(...) === true })), which becomes the repos state via setRepos(validRepos). Because handleToggleRepo, handleRemoveRepo, handleToggleStar, and handleToggleHidden all build their save payload from this same repos state and call performAutoSave with the full array (as shown by handleAddRepo's and handleToggleAutoCiFollowup's identical pattern in the same file), any subsequent save for a duplicate-name repo (e.g. two branch entries where one was deliberately set to false via the CLI/API and another is true) re-persists the aggregated true value onto the entry that was actually stored as false, even though the admin only intended to change an unrelated field.
• Minimum fix: In loadRepos, keep each entry's own persisted autoFollowupOnFailedCi value in the repos state as returned by the API (do not overwrite it with the aggregate). Compute the OR-aggregate separately, purely for display purposes (e.g., a derived value passed to AutoCiFollowupControl), so that saves triggered by unrelated handlers round-trip the true per-entry value instead of the display aggregate.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 CLI repo toggle does not sync duplicate branch-specific entries

updateRepo in packages/cli/src/api/repos.ts matches only the first repository entry with a given name via findIndex. When a repository is monitored multiple times with different baseBranch values (an explicitly supported pattern per AddRepositoryModal.tsx's helper text), propr repo toggle --auto-ci-followup/--no-auto-ci-followup only updates one of the duplicate entries, while the backend's isAutoCiFollowupEnabledForRepository still ORs across all entries sharing that name. This means a CLI user disabling the flag on one branch entry may see no effective change if another entry remains enabled, with no CLI feedback about the other entries. This is optional because it's an edge case (duplicate repos already have this "first match only" limitation for other fields too), but a short note in repo toggle's output when duplicates exist would help.

S2: 🟢 Comment-edit path is not marker-aware

Only processCommentEvent (comment creation) in packages/core/src/webhook/commentEventHandler.ts strips/recognizes the propr:ci-failure-followup marker. If a bot-authored CI follow-up comment is later edited (e.g., by an admin with write access), the edit-processing path doesn't get the same marker-stripping/bypass treatment. This is a narrow edge case outside the PR's core flow and not required by the linked issues, but worth a follow-up for consistency.

S3: 🟢 Stale-head check treats fetch failures the same as a genuinely stale head

In packages/core/src/webhook/checkRunHandler.ts, getCurrentPRHead returning null (on a transient GitHub API error) is treated identically to "PR moved to a newer head," silently skipping the follow-up. Distinguishing a fetch failure from a genuine mismatch (e.g., retrying or logging at a different level) would improve resilience, but this fails safe rather than unsafe, so it's not blocking.

S4: 🟢 Consider adding a regression test for the duplicate-entry save-corruption scenario (F1)

The existing useRepositoryManagement.test.tsx covers handleToggleAutoCiFollowup syncing duplicates and covers handleAddRepo preserving existing values, but no test exercises "load a repo with externally-set divergent per-entry values, then perform an unrelated save (e.g. handleToggleRepo), and assert the untouched entry's autoFollowupOnFailedCi is preserved." Adding this test alongside the F1 fix would guard against regressions.

Score

One real but narrow-scope data-integrity bug (F1) prevents a clean merge-ready rating despite otherwise strong, well-tested implementation and all-green checks.

Score: 5/10

---

🤖 Review Details

• Model: Claude Sonnet 5
• Time: 4m 58s
• Tokens: 119,728 (90,362 in / 29,366 out)
• Cost: $0.52

View Task

---

💡 Next step: Comment /fix to address F# merge blockers only.
F# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. Suggestions require a separate ordinary follow-up request.

_{🤖 Review by ProPR}

[propr-dev]

✅ Applied the requested follow-up changes in commit 4302f2c

Addressed 1 AI review comment (IDs: 5402895295)

Summary of Changes

Implemented F4 only.

• Preserved each entry’s persisted autoFollowupOnFailedCi value in hook state.
• Derived repository-wide OR state solely for list display.
• Ensured unrelated saves retain per-entry values.
• Updated duplicate-branch toggle behavior and added regression coverage.

Verification passed:

• Hook tests: 16/16
• Root and UI TypeScript checks
• Focused ESLint
• git diff --check

---

🤖 Implementation Details

• Model: GPT-5.6 Sol (high)
• Requested By: @propr-ultrafix
• Time: 2m 57s
• Tokens: 340,316 (336,155 in / 4,161 out)
• Cost: $0.44

View Task Execution

---

💡 ProPR Slash Commands

Command	Description	Example
/merge	Merge target branch into this PR and resolve conflicts	/merge
/review	Request an AI code review	/review or /review claude-sonnet
/fix	Implement fixes for issues found by /review	/fix or /fix address the null check issue
/switch	Change the AI model for this PR	/switch claude-opus
/use	Override the model for a single follow-up run	/use claude-sonnet
/ultrafix	Loop review→fix cycles until score goal is met	/ultrafix or /ultrafix goal=8 max=10

_{🤖 Generated by ProPR}

[propr-dev]

✅ AI Code Review Complete requested by @propr-ultrafix

Posted 1 review:

• claude:claude-sonnet-5

View Task Details

[propr-dev]

🔍 AI Code Review — claude:claude-sonnet-5

Overall Evaluation

This PR implements a large, well-scoped feature (repo-level autoFollowupOnFailedCi) spanning core config validation/persistence, webhook detection of failing CI, comment-based agent hand-off, CLI, and UI — matching all three linked issues (#1926, #1927, #1928). I traced the config flow (configRepoValidation.ts → configRoutes.ts → CLI repos.ts/repoCommands.ts → UI useRepositoryManagement.ts), the webhook detection path (checkRunHandler.ts → ciFailureFollowup.ts), and the comment-intake trust boundary (commentEventHandler.ts), and did not find a correctness or safety regression that blocks merge. The default-off behavior is preserved consistently end-to-end, deduplication is implemented with both a Redis NX claim and a marker-based comment scan, and the "omitted vs. explicit false" preservation logic in postRepos is carefully handled and test-verified.

✅ Consistent off-by-default semantics across every layer — normalizeRepoConfig defaults missing values to false (configRepoValidation.ts), getRepos coerces legacy entries to false (configRoutes.ts), and postRepos distinguishes an omitted field from an explicit false using the raw request body rather than the normalized value, correctly preserving prior state (verified against packages/api/test/configRepoRoutes.test.ts).

✅ Narrow, testable trust boundary for the hidden marker — prepareCiFollowupComment only bypasses author filtering when the comment author is in configuredBotUsernames and the marker is present, and this is validated by both a positive and an explicit negative test (test/commentEventHandler.switch-use.test.ts, "does not let an unrelated bot use the CI marker to bypass author filtering").

✅ Stale-SHA guard prevents outdated follow-ups — both handleCheckRunEvent and handleStatusEvent re-fetch the current PR head and skip posting when it no longer matches the failing SHA, directly covered by new tests in test/checkRunHandler.test.ts.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Avoid PR-head lookups for repos with the feature disabled

In packages/core/src/webhook/checkRunHandler.ts, handleCheckRunEvent and handleStatusEvent call getCurrentPRHead (a GitHub API request) for every failing check/status on every monitored PR before postCiFailureFollowup checks isAutoCiFollowupEnabledForRepository. Since the flag is off by default for all existing repos, this adds an unconditional extra GitHub API call for the vast majority of installations on every CI failure. Checking the repo flag first (or passing it through) would avoid unnecessary API-rate-limit consumption without changing behavior. This is an efficiency concern, not a correctness bug, so it is not merge-blocking.

S2: 🟢 Raw CI annotation/output content is not markdown-escaped

buildCiFailureFollowupComment in packages/core/src/webhook/ciFailureFollowup.ts escapes the check name (escapeInlineMarkdown/escapeInlineCode) but embeds annotation titles/messages and fallback summaries verbatim inside a blockquote. Since this content can originate from CI output on externally-triggered runs, and the resulting comment is designed to bypass normal author filtering and feed directly into agent processing, consider escaping or fencing this content as defense-in-depth against markdown/prompt-injection via crafted CI output. This is consistent with the existing trust model (agents already read PR content from external contributors), so it's a hardening suggestion rather than a blocker.

S3: 🟢 action_required conclusion may not represent an actual failure

FAILING_CHECK_RUN_CONCLUSIONS in ciFailureFollowup.ts includes action_required, which GitHub uses for manual-approval gates (e.g., deployment approval) rather than a code defect. Treating it as a "CI failure" could produce a confusing automatic follow-up comment asking the agent to "investigate and fix" something that isn't a bug. Consider excluding it, or scoping the excerpt/instructions differently for that conclusion.

S4: 🟢 Extra comment-scan on top of the Redis dedupe claim

hasExistingFollowupComment performs a full paginated fetch of PR comments on every successful Redis claim as additional insurance against duplicate posts. Given the Redis SET NX claim already provides atomic deduplication for the common case, consider whether this extra API cost is worth retaining unconditionally, or whether it should be reserved for recovery paths (e.g., after a Redis error).

S5: 🟢 Wildcard export from ciFailureFollowup.js

packages/core/src/index.ts adds export * from './webhook/ciFailureFollowup.js';, while the rest of the file uses explicit named exports. As this module's internal API surface grows, a wildcard export increases the risk of accidental symbol collisions with other core exports. Consider switching to explicit named exports for consistency and long-term stability.

Score

No actionable findings were identified after tracing the config, webhook-detection, and comment-intake paths against the stated requirements, and all current-head checks pass; the suggestions above are hardening/efficiency items rather than correctness blockers.

Score: 9/10

---

🤖 Review Details

• Model: Claude Sonnet 5
• Time: 4m 13s
• Tokens: 117,546 (91,519 in / 26,027 out)
• Cost: $0.49

View Task

---

💡 Next step: Comment /fix to address F# merge blockers only.
F# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. Suggestions require a separate ordinary follow-up request.

_{🤖 Review by ProPR}