feat: implement FIXED suggestion regression detection (PR #1999 missing feature)

[absarasw]

Problem

PR #1999 promised to implement two key behaviors but only delivered one:

✅ Delivered: Auto-detect publish state for Fix entities
❌ Missing: Regression detection - create NEW suggestions when FIXED+fully-resolved issues reappear

What happened:
The regression detection logic was present in intermediate commits of PR #1730/#1999:

• 39bcfe66 - had fullyPublishedFixedKeys logic
• 790687c6 - refined the implementation

But when PR #1999 was squash-merged (b24e8fe7), this logic was dropped. The final merge just has a simple existingSuggestionKeys check that blocks all existing keys equally, regardless of Fix entity status.

Current behavior (broken):
When a FIXED+PUBLISHED issue reappears in audit results:

• ❌ No new suggestion created (blocked by existingSuggestionKeys)
• ❌ Existing FIXED suggestion not updated (after PR fix(sync): prevent spurious updatedAt bumps on terminal suggestions #2593's deepEqual optimization)
• ❌ Silent regression - the issue is back but nobody knows

---

Solution

This PR restores the missing regression detection logic with improvements:

1. Skip Updating FIXED Suggestions

FIXED suggestions are never updated when they reappear in audit results:

if (existing.getStatus() === STATUSES.FIXED) {
  log.debug(`Skipping FIXED suggestion - FIXED suggestions are never updated`);
  return; // Don't add to toUpdate array
}

2. Detect Fully Resolved FIXED Suggestions

Before creating new suggestions, query Fix entities to identify FIXED suggestions where ALL fix entities have reached terminal status:

For non-author-only opportunities (e.g., broken-backlinks):

• Check: ALL fix entities have status='PUBLISHED'
• Rationale: Verified on production, the fix has shipped

For author-only opportunities (e.g., security-permissions):

• Check: ALL fix entities have status='DEPLOYED'
• Rationale: Author-only changes can't publish to production, DEPLOYED is terminal

3. Allow Regression Creation

When filtering new suggestions:

if (!existingSuggestionKeys.has(key)) {
  return true; // No existing - create
}

if (fullyResolvedFixedKeys.has(key)) {
  log.warn(`Regression detected: FIXED+${statusType} suggestion found in audit`);
  return true; // Regression - create NEW suggestion
}

return false; // Block otherwise

---

Behavior Matrix

Scenario	Before This PR	After This PR
New issue appears	✅ Create NEW suggestion	✅ Create NEW suggestion
Existing NEW/PENDING updated	✅ Update if changed	✅ Update if changed
FIXED (unpublished) reappears	⚠️ No update, no new suggestion	✅ Skip update, no new suggestion
FIXED (all fixes PUBLISHED) reappears	❌ No update, no new suggestion (silent regression)	✅ Create NEW suggestion

---

Scope

✅ Applies to: syncSuggestionsWithPublishDetection only
❌ Does not apply to: Base syncSuggestions (unchanged)

This matches the original intent - regression detection is a publish-detection feature, not a base sync behavior.

---

Testing Strategy

Unit Tests (TODO - will add after implementation review)

1. FIXED skip test: Verify FIXED suggestions are not updated
2. Regression detection (non-author-only): FIXED+PUBLISHED → NEW suggestion created
3. Regression detection (author-only): FIXED+DEPLOYED → NEW suggestion created
4. No regression when not fully resolved: FIXED with mixed DEPLOYED/PUBLISHED → no new suggestion

Integration Testing

• Broken-backlinks audit (non-author-only path)
• Security-permissions audit (author-only path)

---

Performance Considerations

Fix entity queries: This adds N async database calls (one per FIXED suggestion).

Mitigation strategies considered:

• ✅ Only queries FIXED suggestions (typically small subset)
• ✅ Uses Promise.all for parallel execution
• ✅ Only runs in syncSuggestionsWithPublishDetection (not base syncSuggestions)
• 🔄 Future: Batch query for fix entities (if N grows large)

---

Related PRs

• fix(sync): prevent spurious updatedAt bumps on terminal suggestions #2593 - deepEqual optimization (prevents spurious updatedAt bumps) - Companion PR
• feat: Adding auto-detect for publish during broken backlinks audits run #1999 - Original PR that promised this feature but didn't deliver it
• feat: Adding auto-detect for publish during broken backlinks audits run #1730 - Initial implementation that had the logic before feat: Adding auto-detect for publish during broken backlinks audits run #1999 replaced it

References

• JIRA: SITES-44646 - Spurious updatedAt bumps
• PR #1730 commits - Original implementation
• Commit 39bcfe66 - Had fullyPublishedFixedKeys logic

---

Merge Order

Can be merged in any order with PR #2593. They are independent changes:

• This PR: FIXED regression detection (only in syncSuggestionsWithPublishDetection)
• PR fix(sync): prevent spurious updatedAt bumps on terminal suggestions #2593: deepEqual optimization (applies to both syncSuggestions and syncSuggestionsWithPublishDetection)

Recommended order: Merge #2593 first (simpler change), then this PR.

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot]

Hey @absarasw,

Strengths

• src/utils/data-access.js:894 - The FIXED skip logic with early return is clean and well-placed. Prevents unnecessary data merging for resolved suggestions.
• src/utils/data-access.js:910 - The deepEqual optimization to avoid writing unchanged suggestions is a genuine improvement preventing spurious updatedAt bumps.
• src/utils/data-access.js:940-950 - Regression detection design is sound: requiring both non-empty fix entities AND all-terminal-status before declaring a regression prevents false positives. A FIXED suggestion with no fix entities (data inconsistency) will not trigger false regressions.
• The decision to inline the update logic rather than adding more callback parameters to syncSuggestions keeps each variant self-contained and avoids over-parameterizing the base function.
• No new security surface - the change operates entirely within trusted boundaries (SQS -> Lambda -> DB) with fail-closed error handling in the regression detection path.

Issues

Critical (Must Fix)

1. handleOutdatedSuggestions called with wrong parameters - will break at runtime
src/utils/data-access.js:1051

The PR passes:
```javascript
await handleOutdatedSuggestions({
disappearedSuggestions,
statusToSetForOutdated,
scrapedUrlsSet,
log,
});
```

But handleOutdatedSuggestions expects { context, existingSuggestions, newDataKeys, buildKey, statusToSetForOutdated, scrapedUrlsSet }. It uses context.dataAccess internally to get Suggestion and context.log for logging, and computes disappeared suggestions by filtering existingSuggestions against newDataKeys using buildKey.

The function will receive undefined for context, causing a throw on context.dataAccess. No suggestions will ever be marked OUTDATED in syncSuggestionsWithPublishDetection.

Fix: Pass the correct parameters:
```javascript
await handleOutdatedSuggestions({
context,
existingSuggestions,
newDataKeys,
buildKey,
statusToSetForOutdated,
scrapedUrlsSet,
});
```

2. Default merge/status functions diverge from base syncSuggestions
src/utils/data-access.js:900

```javascript
const mergedData = mergeDataFunction
? mergeDataFunction(existingData, newDataItem)
: { ...existingData, ...newDataItem };
```

In base syncSuggestions, mergeDataFunction defaults to defaultMergeDataFunction at the function signature level. Similarly, mergeStatusFunction defaults to defaultMergeStatusFunction which handles REJECTED->keep, OUTDATED->regression, and ERROR->NEW transitions. The inlined version defaults to returning null (no status change), silently dropping these transitions.

If any caller of syncSuggestionsWithPublishDetection relies on these default transitions, behavior silently diverges from what the base function would do.

Fix: Apply the same defaults:
```javascript
const effectiveMergeData = mergeDataFunction ?? defaultMergeDataFunction;
const effectiveMergeStatus = mergeStatusFunction ?? defaultMergeStatusFunction;
```

Important (Should Fix)

3. Missing tests - 100% coverage requirement violated
src/utils/data-access.js:875-1057

The repo's CLAUDE.md states: "Coverage enforcement is strict: 100% lines, branches, and statements across all src/**/*.js. Every code path must be tested." The PR adds ~167 lines of production code with 12+ branch paths and zero test files. CI is failing. The PR body acknowledges this ("Unit Tests (TODO - will add after implementation review)").

Fix: Add unit tests covering at minimum:

• FIXED suggestion skip logic
• Regression detection (both author-only/DEPLOYED and non-author-only/PUBLISHED paths)
• Partial resolution (not all fixes at terminal status)
• Fix entity query failures
• deepEqual change detection
• New suggestion status determination (PENDING_VALIDATION vs NEW)
• Partial and total addSuggestions failures

4. Silent error swallowing in fix entity queries
src/utils/data-access.js:948

The catch block logs at debug level. If getFixEntitiesBySuggestionId fails systematically (DB connectivity, missing table), ALL FIXED suggestions silently skip regression detection. The function completes successfully with fullyResolvedFixedKeys empty, meaning regressions are silently missed - which directly undermines the PR's stated goal.

Fix: Count failures and escalate when all queries fail:
```javascript
let failCount = 0;
// ...inside catch:
failCount++;
// ...after Promise.all:
if (failCount > 0 && failCount === fixedSuggestions.length) {
log.warn(`All ${failCount} fix entity checks failed - regression detection incomplete`);
}
```

5. Unbounded concurrent database calls in Step 4
src/utils/data-access.js:931

Promise.all(fixedSuggestions.map(...)) fires one getFixEntitiesBySuggestionId call per FIXED suggestion with no concurrency limit. FIXED suggestions accumulate monotonically over a site's lifetime. A site audited for 2+ years could have dozens to hundreds of FIXED suggestions on a single opportunity, creating unbounded fan-out against the database.

Fix: Use a bounded concurrency approach (batch processing in chunks of 10, or a concurrency-limiting utility). Alternatively, if the data-access layer supports it, add a batch query that fetches fix entities for multiple suggestion IDs in one round-trip.

6. Code duplication creates divergence surface
src/utils/data-access.js:875-1057

Steps 3, 5, and 6 now carry their own copy of update logic, new-suggestion creation, error handling, and outdated-suggestion handling. When base syncSuggestions evolves (new validation, bug fixes), the publish-detection variant will silently drift unless both are patched in lockstep.

Fix: Consider a strategy-pattern extension: syncSuggestions could accept optional shouldSkipUpdate(existing) and allowCreationOverride(key, existingSuggestionKeys) predicates. This keeps the behavioral extension isolated while the mechanical plumbing stays in one place.

Recommendations

• The isAuthorOnly boolean for terminal-status branching (DEPLOYED vs PUBLISHED) may grow as the platform adds opportunity types with different lifecycle models. Consider whether this should become a status-resolution function supplied by each audit type rather than accumulating more boolean parameters.
• Structure tests around the 6 steps with describe blocks per step. Test the interaction between Step 3 (skipping FIXED in update) and Step 4 (using those same FIXED suggestions for regression detection).

Assessment

Ready to merge? No
Reasoning: The handleOutdatedSuggestions call passes incorrect parameters and will throw at runtime, breaking outdated-suggestion detection entirely. The merge/status function defaults diverge from the base, risking silent behavioral differences. CI is failing due to missing test coverage (a documented 100% requirement). These must be fixed before merge.

Note: CI checks are currently failing - this should be resolved before merge.

Next Steps

1. Fix the handleOutdatedSuggestions call to pass the correct parameters (Critical).
2. Apply proper defaults for merge/status functions to match base syncSuggestions behavior (Critical).
3. Add unit tests for all branch paths to satisfy the 100% coverage requirement and fix CI.
4. Add failure counting for the fix entity query catch block so systematic failures are visible.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 22s | Cost: $3.16 | Commit: 92476a4703cf280696727f65dde7c30b89817776
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot]

Hey @absarasw,

Verdict: Request changes - two correctness/resilience issues need fixing before this safely detects regressions in production.
Changes: implements FIXED-suggestion regression detection by inlining the sync loop with a FIXED-skip, querying fix entities to identify fully-resolved suggestions, and allowing new suggestion creation for regressions (2 files).

Must fix before merge

1. [Important] isTBYB not passed to mergeStatusFunction - silent behavioral divergence from base syncSuggestions - src/utils/data-access.js:908 (details inline)
2. [Important] Silent error swallowing in fix entity queries - regression detection silently skipped with no operator signal - src/utils/data-access.js:965 (details inline)
3. [Important] Unbounded concurrent database calls in Step 4 - no concurrency limit despite existing limitConcurrencyAllSettled pattern - src/utils/data-access.js:945 (details inline)

Non-blocking (4): minor issues and suggestions

• nit: effectiveMergeData and effectiveMergeStatus are loop-invariant but recomputed inside forEach - hoist above the loop - src/utils/data-access.js:902
• nit: suggestions.createdItems?.length <= 0 is misleading - if createdItems is undefined, optional chaining yields undefined <= 0 which is false, skipping the throw even when zero items were created - use !suggestions.createdItems?.length instead - src/utils/data-access.js:1038
• suggestion: partition matchedSuggestions into fixed/non-fixed upfront rather than filtering FIXED in the forEach (Step 3) and re-filtering the same array in Step 4 - src/utils/data-access.js:880
• suggestion: extract Step 4 (regression-eligibility check) into a named function to isolate the concurrency fix and improve testability - src/utils/data-access.js:931

Previously flagged, now resolved

• handleOutdatedSuggestions called with correct parameters (commit d4bcc0f)
• Default merge/status functions now use defaultMergeDataFunction/defaultMergeStatusFunction via nullish coalescing (commit 9850794)
• Test coverage meets 100% requirement (Codecov confirmed)

Note: CI checks are currently pending - resolve before merge.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 48s | Cost: $4.63 | Commit: 56687cf13b441f80914a2a5af3864556ade04083
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): mergeStatusFunction receives bare context instead of { ...context, isTBYB }.

In base syncSuggestions (line 507), isTBYB is computed via checkIsTBYBSite(context) and passed as { ...context, isTBYB } to mergeStatusFunction. The inlined version here passes bare context, so defaultMergeStatusFunction sees context.isTBYB as undefined (falsy). For PLG/freemium sites, this means OUTDATED suggestions that reappear will get PENDING_VALIDATION instead of NEW, diverging from base behavior.

checkIsTBYBSite is already called at line 848 (Step 1). Store its result and pass { ...context, isTBYB } here:

const newStatus = effectiveMergeStatus(existing, newDataItem, { ...context, isTBYB });

Also use it for defaultNewSuggestionStatus computation in Step 5 (line 982).

[MysticatBot]

issue (blocking): The catch block logs at debug level. If getFixEntitiesBySuggestionId fails (DB throttling, timeout, permissions), the suggestion is silently excluded from fullyResolvedFixedKeys - regression detection simply does not fire for that suggestion, with no operator-visible signal.

This is the core feature of this PR. A degraded database that causes all queries to fail means zero regressions are detected and no one knows.

Fix: log at warn to match the pattern used in publishDeployedFixEntities (line 789) and reconcileDisappearedSuggestions (line 675) for the same class of failure:

log.warn(`Failed to check fix entities for suggestion ${suggestion.getId()}, regression check skipped: ${e.message}`);

[MysticatBot]

issue (blocking): Promise.all(fixedSuggestions.map(...)) with no concurrency limit. This file already uses limitConcurrencyAllSettled with MAX_CONCURRENT_CHECKS = 5 for the same fan-out pattern in reconcileDisappearedSuggestions (line 634) and publishDeployedFixEntities (lines 765, 772). A site accumulating FIXED suggestions over time creates unbounded burst load.

Fix: use the existing utility:

const fixEntityTasks = fixedSuggestions.map((suggestion) => async () => {
  // ... existing logic
});
await limitConcurrencyAllSettled(fixEntityTasks, MAX_CONCURRENT_CHECKS);