Skip to content

fix(observability): surface publish/checks-perm/secret-scan failures to Sentry#1627

Merged
JSONbored merged 1 commit into
mainfrom
fix/review-failures-to-sentry
Jun 27, 2026
Merged

fix(observability): surface publish/checks-perm/secret-scan failures to Sentry#1627
JSONbored merged 1 commit into
mainfrom
fix/review-failures-to-sentry

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Surfaces three review-pipeline failure classes in processors.ts to Sentry. The self-host forwarder (src/selfhost/sentry.ts) only forwards level:"error"/"fatal" console lines, so these were invisible to operators:

  1. Total public-surface publish failure (maybePublishPrPublicSurface, the publishedOutputs.length === 0 && failedOutputs.length > 0 branch): the advisory ran but nothing reached the PR (revoked token / perms removed / GitHub 5xx) — the worst failure for an advisory-only bot — was recorded only to the DB audit ledger. Now escalated via captureReviewFailure(..., { kind:"publish", owner, repo, pr, head_sha, failedOutputs }) (already imported; no-op when Sentry is off). Scoped to the total-failure branch (a partial publish stays ledger-only to avoid noise).
  2. Install-wide Checks:write gap (auditGateCheckPermissionMissing chokepoint + the context check-run branch): a common self-host misconfig silently prevents the required gate check-run from ever posting on any PR — now logged at level:"error" (gate_check_permission_missing / check_run_permission_missing). Volume bounded (once-per-install config fault).
  3. Secret-leak scan crash (maybeAddSecretLeakFinding catch): a crashed credential scanner degraded silently to "no finding" at level:"warn" — flipped to level:"error" (a security-observability event).

Surfaced by the 2026-06-27 review-pipeline hardening audit (observability — high + medium).

Scope

Validation

  • git diff --check · actionlint · typecheck
  • test:coverage — all three branches are exercised by existing tests (pr_public_surface_failed, gate_check_permission_missing, check_run_permission_missing); the new log/capture lines add no branches. Added a captureReviewFailure spy to the existing total-failure test pinning the kind:"publish" escalation. The secret-scan catch is /* v8 ignore */.
  • test:workers · build:mcp · test:mcp-pack · ui:* · npm audit --audit-level=moderate

If any required check was skipped, explain why:

  • No migration/OpenAPI/cf-typegen: logging/escalation only.

Safety

  • No secrets/wallets/trust-scores exposed — logs carry repo/PR/SHA + bounded fields.
  • No public GitHub text change · auth/CORS N/A.

Notes

  • Self-host engine code; ships on the next engine rebuild. captureReviewFailure is a no-op on cloud/Workers and on self-host without a Sentry DSN.

…to Sentry

Three review-pipeline failure classes in processors.ts were invisible to the
self-host Sentry forwarder (which only forwards level:error/fatal):

- Total public-surface publish failure (maybePublishPrPublicSurface): the
  advisory ran but NOTHING reached the PR (revoked token / perms removed / 5xx)
  was recorded only to the DB audit ledger. Escalate via captureReviewFailure
  (kind:publish), scoped to the total-failure branch (partial publish stays
  ledger-only to avoid noise).
- Install-wide Checks:write gap (auditGateCheckPermissionMissing chokepoint +
  the context check-run branch): a misconfig that silently stops the required
  gate check-run from ever posting now logs at level:error.
- Secret-leak scan crash: a crashed credential scanner degraded silently at
  level:warn -> flipped to level:error.

Covered by existing pr_public_surface_failed / *_permission_missing tests; a
captureReviewFailure spy pins the total-failure escalation.
@dosubot dosubot Bot added the size:S label Jun 27, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.52%. Comparing base (0cf3b30) to head (f064c6e).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1627   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files         204      204           
  Lines       22056    22060    +4     
  Branches     7967     7967           
=======================================
+ Hits        21069    21073    +4     
  Misses        412      412           
  Partials      575      575           
Files with missing lines Coverage Δ
src/queue/processors.ts 88.32% <100.00%> (+0.03%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit e4a508b into main Jun 27, 2026
18 checks passed
@JSONbored
JSONbored deleted the fix/review-failures-to-sentry branch June 27, 2026 23:32
JSONbored added a commit that referenced this pull request Jun 28, 2026
…the event slug (#1636)

forwardStructuredLogToSentry titled each issue with only obj.event and buried
the real failure in a 'log' context blob — so operators had to open each issue
to learn what broke. Now:
- title leads with the failure: "<event>: <message ?? error>" (e.g.
  orb_broker_unavailable: The operation was aborted due to timeout)
- index filterable tags for the dimensions operators search/group by (repo,
  installationId, pull, pr, project, kind, deliveryId), present values only
- fingerprint by event so recurrences collapse into one issue instead of
  fragmenting on the variable detail now in the title

Pairs with the source-side instrumentation (#1605/#1619/#1625/#1627/#1628):
those make failures reach Sentry; this makes each issue legible at a glance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant