Skip to content

Omit config values from write failure diagnostics - #541

Merged
selviarora merged 1 commit into
Shopify:mainfrom
selviarora:fix/config-write-error-values
Sep 15, 2026
Merged

selviarora merged 1 commit into
Shopify:mainfrom
selviarora:fix/config-write-error-values

Conversation

@selviarora

Copy link
Copy Markdown
Contributor

When a config write fails, ConfigWriteError currently includes old and new values in its message. This sends credentials to stderr and configured exception reporters during initial storage, rotation, or deletion. The exception also retains the complete old/new config contents, including unchanged values.

Keep the section/key diagnostic and addition/removal markers, but omit every value and retain only the sanitized diff on the exception. Compare the attempted in-memory config directly so multiline values cannot be reparsed as diagnostic key or section names. Atomic writes, symlink handling, errno preservation, and SystemCallError rescue compatibility remain covered.

ConfigWriteError now takes parsed config hashes instead of serialized strings and no longer exposes old_content or new_content; its config_path, diff, and errno readers remain available.

Validation on Ruby 3.4.3 with the locked dependencies:

  • bundle exec rake test: 148 runs, 691 assertions, 0 failures/errors, 7 existing skips.
  • bundle exec rake style: 62 files, no offenses.
  • bundle exec srb tc: no errors.
  • Five new regression tests fail against the original implementation and pass with the fix. Coverage includes initial storage, rotation, deletion, multiline values, stderr, and reporter delivery; the unchanged-value test also checks serialized exception state.
  • A separate reproduction using real read-only temporary directories confirms values are absent from messages, stderr, serialized exceptions, and an in-process reporter invoked at exit.

Assisted-By: devx/736bac94-4b2a-4b19-b5cc-1826ad565f21
@selviarora
selviarora merged commit 91a166a into Shopify:main Sep 15, 2026
9 checks passed
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