refactor(skills)!: remove the GitLab die() helper in favor of GitLabError

[Jamie Kim (jkim323)]

Pull Request

Description

Removes the die() helper from the GitLab skill so GitLabError becomes the module's single failure mechanism, and adds a source contract so the boundary cannot drift again.

GitLabError is the module-wide typed failure base. GitLabAPIError(GitLabError) is the API-specific subclass used for transport failures; non-transport validation, configuration, and command errors remain GitLabError.

This closes the last open acceptance criterion of #1555. The other seven criteria — LOGGER, the _emit output sink, _emit_debug_traceback, typed API error classes, the source-contract tests, and the gitlab_token global removal — already landed in #2718 for both the Jira and GitLab skills. No Jira changes are needed.

Why this is not cosmetic

die() was already redaction-safe: it routed through _emit(), so the raw-stderr leak described in the issue was fixed by #2718. The remaining problem was an internal contract violation. The GitLabError docstring stated that die() survives only for "the argument-parsing and command-dispatch layer, where no value is promised" — yet die() was being called from four helpers that do promise return values:

Helper	Returns
_auth_headers	dict[str, str]
_required_oauth_client_id	str
project	str
_read_capped	bytes

That is exactly the "silent None return" hazard the docstring warns against. The documented contract and the code disagreed, and nothing enforced the split.

What changed

• All 61 die() call sites now raise GitLabError(message, exit_code), preserving each original message and exit-code constant. Sites inside except blocks chain with from exc.
• die() is deleted; the now-unused NoReturn import is dropped.
• GitLabError's docstring is rewritten to describe the single-mechanism contract.
• 42 SystemExit test references are migrated across 7 files.
• New source contract: no raise SystemExit, no sys.exit(...), and no die definition outside the __main__ guard — with one negative case per predicate.

Behavior

Exit codes are unchanged. main() already caught GitLabError, emitted once via _emit(), and returned exc.exit_code, so no new error handling was introduced.

Two deliberate consequences, both improvements:

1. Library helpers now raise without emitting. Emission happens once, at the main() boundary, removing a double-emission path.
2. SystemExit is a BaseException and previously bypassed main() entirely. These failures now flow through main(), so GITLAB_DEBUG=1 shows a redacted traceback for argument-validation failures that previously printed only a one-line error.

Related Issue(s)

Closes #1555

Type of Change

Code & Documentation:

• Breaking change (fix or feature causing existing functionality to change)

Other:

• Script/automation (.ps1, .sh, .py)

die() was a public module attribute, so its removal is technically breaking for any external caller importing gitlab.die. The CLI surface, exit codes, and output format are unchanged.

Testing

Test migration

The migration was not a uniform substitution. Sites fell into three kinds:

Kind	Migration
Direct-call	pytest.raises(gitlab.GitLabError), .code to .exit_code, and stderr assertions moved to str(exc_info.value) because converted helpers no longer emit
main() boundary	pytest.raises removed entirely; assert gitlab.main() == <constant> plus emitted stderr
Already typed	Untouched

No assertion was weakened to a bare pytest.raises(Exception). Every migrated site that checked an exit code still checks the same constant.

TestDie was removed. Its exact error: boom single-line assertion was not already covered elsewhere — test_emit_writes_exactly_one_stderr_line exercises _emit directly, and no test previously reached main()'s handler because SystemExit bypassed it. A replacement assertion was added to a converted main() test:

assert capsys.readouterr().err == "error: --fields is not valid with auth commands\n"

Validation status

Command	Result
npm run lint:py	Pass
npm run validate:skills	Pass — 74 skills, 0 errors, 0 warnings
npm run test:py	Blocked — environmental, not a code failure

npm run test:py could not execute locally. uv sync fails with os error 32 inside the OneDrive-synced workspace and os error 396 outside it, PyPI is unreachable with a TLS HandshakeFailure, and the existing skill virtualenvs hold a partially installed pytest. This predates the change and affects unrelated skills identically.

Because the suite could not run, acceptance criteria were verified directly against system Python: die is absent from the module, the source contract rejects all three synthetic violations and accepts the guarded sys.exit(main()), and no SystemExit reference remains outside the contract's own test fixtures.

Reviewers: please confirm the GitLab suite passes in CI before merging. That is the one gate not satisfied locally.

Checklist

Required Checks

• Documentation is updated (if applicable) — n/a, no doc-visible behavior change
• Files follow existing naming conventions
• Changes are backwards compatible (if applicable) — see Type of Change
• Tests added for new functionality (if applicable)

Required Local Checks

• npm run validate:local
• npm run spell-check
• npm run lint:md-links

Security Considerations

• This PR does not contain any sensitive or NDA information
• Any new dependencies have been reviewed for security issues
• Security-related scripts follow the principle of least privilege

No new dependencies. No change to _redact, _REDACT_KEYS, _REDACT_PATTERNS, the emit helpers, AuthContext, the OAuth modules, or the audit sink. The existing print-ownership, LOGGER.exception, and two-owner credentialed-egress contracts pass unmodified.

Additional Notes

The path:line references in #1555 are stale — the skills moved from .github/skills/{jira,gitlab}/{jira,gitlab}/ to .github/skills/project-planning/{jira,gitlab}/ and both files roughly tripled in size. Worth correcting the issue body on close.

The PR validation failure on Asset Docs Validation was due to the shared PowerShell module installer assuming PSGallery was already registered on the runner. The installer now self-registers PSGallery on demand, and the fix is covered by focused tests in scripts/tests/security/Install-PSModules.Tests.ps1.

[github-actions]

Eval Execution

✅ Status: Passed

• Artifacts evaluated: 0
• Specs run: 0
• Assertions passed: 0
• Assertions failed (blocking): 0
• Assertions failed (advisory): 0
• Failed specs (merge-blocking): 0

No changed AI artifacts required evaluation.

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 76.05634% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.26%. Comparing base (3c3dc02) to head (862942f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...b/skills/project-planning/gitlab/scripts/gitlab.py	73.84%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
+ Coverage   82.98%   84.26%   +1.27%     
==========================================
  Files         183       83     -100     
  Lines       33787    11240   -22547     
  Branches       25        0      -25     
==========================================
- Hits        28038     9471   -18567     
+ Misses       5746     1769    -3977     
+ Partials        3        0       -3     

Flag	Coverage Δ
docusaurus	?
pester	83.84% <100.00%> (+0.38%)	⬆️
pytest	87.69% <73.84%> (+4.94%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
scripts/security/Install-PSModules.ps1	96.77% <100.00%> (+0.34%)	⬆️
...b/skills/project-planning/gitlab/scripts/gitlab.py	89.02% <73.84%> (+1.63%)	⬆️

... and 103 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Katrien De Graeve (katriendg)]

Thank you. The failure-mechanism migration is consistent, the GitLab suite and repository checks pass, and the breaking API removal is disclosed. I found three medium follow-ups: tighten the AST contract so non-equality and else exits cannot bypass it; prevent provider-controlled OAuth causes from escaping redaction for direct helper consumers; and reconcile issue #1555's GitLabAPIError wording with the GitLabError base type used here before closing the issue.

Approving, as comments need to be closed before merge, and I know you do that!

[Katrien De Graeve (katriendg)]

The suite passes 283 tests, but the PR evidence does not report the measured line or branch coverage for this GitLab skill. The apparent coverage level of roughly 70% is too low for a security-sensitive CLI boundary that owns OAuth, credential handling, redaction, audit fail-closed behavior, and transport validation. Please run the skill suite with coverage, publish the missing-line report, and add tests for the highest-risk uncovered branches. If the current value is near 70%, please raise it materially (at least above 80%, or to the repository-defined threshold if higher) and enforce the agreed floor in CI so it cannot regress. Branch coverage for the newly changed error and exception paths would be especially useful.