fix: resolve safe-settings sync workflow failures - #186
Conversation
gxmiranda
left a comment
There was a problem hiding this comment.
LGTM. Three-bug fix for the safe-settings sync workflow: regex-to-glob pattern correction, exclude-to-include scoping rewrite, and a pragmatic check_suite crash workaround with clear upstream tracking (TODO(safe-settings-818)). The include-based scoping is a cleaner design than the previous exclude approach. All CI checks pass.
One minor note on the error string matching in the workaround — see inline comment.
This review was generated by /review-pr (AI-assisted).
| // In full-sync mode (CLI/GHA), handleResults tries to | ||
| // access payload.check_run.check_suite which does not | ||
| // exist outside the webhook flow. The sync completes | ||
| // and results are logged; only the Check Run reporting |
There was a problem hiding this comment.
[MEDIUM] Consider tighter error matching for the check_suite workaround
String(error).includes('check_suite') would also match any unrelated error whose message happens to contain that substring. A more specific match like String(error).includes("Cannot read properties of undefined (reading 'check_suite'") would reduce the risk of silently swallowing a genuine failure.
Low risk given the narrow execution context (only during syncInstallation), but a low-cost hardening. No action required before merge.
There was a problem hiding this comment.
Thanks @gxmiranda . The workflow is still in test but I will prepare this hardening suggestion in a separate PR once we confirm the workflow is stable.
Fix two issues in the safe-settings restrictedRepos configuration: 1. deployment-settings.yml used regex-style patterns (^repo$) but safe-settings uses glob/minimatch for pattern matching, not regex. The anchored patterns never matched any repo names, which would cause an unscoped full sync to skip all repos. Replace with plain names that minimatch handles correctly for exact matching. 2. The scoped deployment-settings generation (repos input) built a restrictedRepos.exclude list from peribolos.yaml. Repos not in peribolos (e.g. nunya, roadmap) were missing from the exclude list, causing safe-settings to process repos it should not manage. Replace with a restrictedRepos.include allowlist built directly from the target repos input, matching the approach used by the base deployment-settings.yml. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
safe-settings v2.1.18 crashes in full-sync mode (GHA/CLI) with: TypeError: Cannot read properties of undefined (reading "check_suite") The handleResults function in nop mode unconditionally accesses payload.check_run.check_suite, which only exists in the webhook flow. The sync itself completes successfully; only the Check Run reporting fails. Replace "npm run full-sync" with a patched script that catches the check_suite TypeError specifically and treats it as non-fatal, while preserving error handling for all other failure modes. This is a temporary workaround. The upstream fix is tracked at: - Issue: github-community-projects/safe-settings#818 - Fix PR: github-community-projects/safe-settings#1018 - Search: TODO(safe-settings-818) in the workflow file Update MAINTAINING.md troubleshooting section with the workaround details and links for tracking when to remove it. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
4ae6b41 to
bada23f
Compare
Narrow the error guard from a broad 'check_suite' substring match to the exact upstream error message: "Cannot read properties of undefined (reading 'check_suite')". This prevents accidentally swallowing an unrelated error whose message happens to contain that substring. Addresses review feedback from @gxmiranda on PR complytime#186. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
Narrow the error guard from a broad 'check_suite' substring match to the exact upstream error message: "Cannot read properties of undefined (reading 'check_suite')". This prevents accidentally swallowing an unrelated error whose message happens to contain that substring. Addresses review feedback from @gxmiranda on PR #186. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
Summary
Fix three bugs causing all Safe Settings Sync workflow runs to fail with exit code 1.
Root cause analysis: Workflow run #4 logs
Bugs Fixed
1.
restrictedRepospatterns use regex syntax but safe-settings uses glob/minimatchdeployment-settings.ymlused regex-style patterns ("^complyctl$") but safe-settingsmatches via
minimatch(glob),not regex. The
^and$anchors have no special meaning in minimatch, so none of thepatterns ever matched — an unscoped full sync would skip all managed repos.
Fix: Replace with plain names (
complyctl,community, etc.) which minimatch handlescorrectly for exact matching.
2. Scoped deployment-settings leaks unmanaged repos
The "Generate scoped deployment-settings" step built a
restrictedRepos.excludelist fromperibolos.yaml. Repos not in peribolos (nunya,roadmap) were missing from the excludelist, causing safe-settings to process repos it should not manage.
From the workflow logs:
Fix: Replace with a
restrictedRepos.includeallowlist built directly from thereposinput, matching the approach used by the base
deployment-settings.yml.3.
check_suiteTypeError crashes full-sync mode (upstream bug)safe-settings v2.1.18 crashes in full-sync mode with:
The
handleResultsfunction in nop mode unconditionally accessespayload.check_run.check_suite, which only exists in the webhook flow. The syncitself completes; only the Check Run reporting fails.
This is a known upstream bug:
Fix: Replace
npm run full-syncwith a patched script that catches thecheck_suiteTypeError specifically and treats it as non-fatal, while preserving error handling for all
other failure modes. Tracked via
TODO(safe-settings-818)for removal once the upstreamfix lands.
Verification
The GitHub App permissions were confirmed correct:
{ "administration": "write", "contents": "read", "metadata": "read", "organization_administration": "write" }Recommended post-merge validation
dry-run: true,repos: complytime-demos(single repo, minimal scope)Allowing complytime-demos in restrictedRepos.includeAllowing nunya/Allowing roadmaplines appearcheck_suite reporting skippedmessage appearsreposempty,dry-run: true)Changes
safe-settings/deployment-settings.yml.github/workflows/safe_settings_sync.ymlinclude+ addcheck_suiteworkaroundMAINTAINING.md