Skip to content

Improve GitHub API error diagnostics, CLI error handling, and spec isolation#389

Merged
ydesgagn merged 1 commit into
masterfrom
fix/api-error-body-and-cli-negative-tests
May 17, 2026
Merged

Improve GitHub API error diagnostics, CLI error handling, and spec isolation#389
ydesgagn merged 1 commit into
masterfrom
fix/api-error-body-and-cli-negative-tests

Conversation

@ydesgagn
Copy link
Copy Markdown
Contributor

Summary

Addresses the two remaining High-severity findings from the deep code review (BUG-004, TEST-005) and fixes a filesystem-isolation defect uncovered while verifying them: the unit suite deleted the repo's own root linter configs when run from the repo root, because LinterJobBuilder operates on relative paths in the CWD and several linter_job_builder_spec contexts let File.exist? call through without stubbing File.delete.

Key changes:

  • GitHubAPIClient now raises a typed GHB::GitHubAPIError that includes a truncated (1000-char) response body, so GitHub's actionable message/errors[] detail is no longer discarded on 4xx/5xx (BUG-004).
  • Application#configure_options broadens its rescue from OptionParser::InvalidOption to the parent OptionParser::ParseError (now also handles MissingArgument/AmbiguousOption) and writes to STDERR via warn instead of STDOUT.
  • Options#args_from_file wraps Shellwords.split so a corrupted persisted args comment raises a clear ConfigError instead of a raw stack trace; empty --excluded_folders entries are dropped.
  • Added negative CLI-parsing specs (unknown flag, missing argument, empty/partial --excluded_folders, malformed quoting) and API-client specs asserting response-body inclusion and 1000-char truncation (TEST-005).
  • Added a filesystem safety-net before hook in linter_job_builder_spec that defaults File.delete/FileUtils.ln_s to no-ops, stopping the suite from destroying the repo's root linter configs; inner allow/have_received expectations are unaffected.

Full suite: 321 examples, 0 failures; line coverage 94.24%, branch 84.73%. RuboCop clean on all changed files. The four root linter configs now survive a full rspec run.

Types of changes

  • Bugfix (fixes an issue)
  • New feature (adds functionality)
  • Refactoring (improves code without changing functionality)
  • Breaking change (incompatible changes)
  • Build or security update (updates dependencies, libraries, or security patches)
  • Code style or documentation update (formatting, renaming, or documentation changes)
  • Other (please describe):

Checklist

  • Unit tests added to validate my fix/feature
  • I have manually tested my change
  • I did not add automation test. Why ?:
  • Database changes requiring migration with downtime or reprocessing of existing data
  • The SOUP file lists the risk Level, requirements and verification reasoning associated with each library
  • `readme.md` includes sections on introduction, installation, usage, and contributing
  • `docs/architecture.md` includes sections on the architecture diagram, software units, software of unknown provenance, critical algorithms and risk controls related to PII and security
  • Impact on PII, privacy regulations (CCPA/GDPR/PIPEDA), CIS benchmarks or security (availability/confidentiality/integrity); management must be notified

@ydesgagn ydesgagn requested a review from a team as a code owner May 17, 2026 18:10
@ydesgagn ydesgagn merged commit bde2875 into master May 17, 2026
16 checks passed
@ydesgagn ydesgagn deleted the fix/api-error-body-and-cli-negative-tests branch May 17, 2026 19:44
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.

2 participants