skip converting empty yaml objects to json

[ashwgit]

PTAL, this one skips empty YAML documents during FBC YAML→JSON conversion to prevent emitting null objects that breaks opm validate while performing add operations.
Also fixes a bug for including conversion for .yml file too.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:48 AM UTC · Completed 10:55 AM UTC
Commit: 45f7743 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [edge-case] iib/workers/tasks/fbc_utils.py:169 — When a non-empty YAML file contains only empty documents (e.g., a file with just ---), every chunk will be None and the continue will skip them all. The output file is opened in append mode but nothing is written, leaving behind a 0-byte .json file. The original YAML file is then deleted. This could cause downstream failures (e.g., opm validate or JSON parsers expecting valid content). The 0-byte input guard does not catch this case.
  Remediation: After the for chunk in data loop completes, check whether anything was actually written (e.g., track a boolean wrote_any). If nothing was written, either raise IIBError or delete the empty output file.

• [scope-inconsistency] iib/workers/tasks/fbc_utils.py:162 — The PR introduces two different approaches to empty content: 0-byte files raise IIBError (fail-fast), while empty YAML documents within a file are silently skipped (fail-safe). The PR description focuses only on the skip behavior. See also: [edge-case] finding at this location.
  Remediation: Clarify why 0-byte files error while empty documents are silently skipped. Document whether both behaviors are intentional.

Low

• [edge-case] iib/workers/tasks/fbc_utils.py:162 — A YAML file that contains only whitespace or only comments (non-zero size) will also yield None chunks from yaml.load_all, producing an empty JSON file. The os.path.getsize == 0 check does not catch semantically-empty files.

• [test-adequacy] tests/test_workers/test_tasks/test_fbc_utils.py:303 — No test covers the edge case where a YAML file is non-empty but contains only empty documents (e.g., ---\n---\n). No test covers a 0-byte .yml file.

• [docstring-convention] iib/workers/tasks/fbc_utils.py:156 — The :raises docstring uses uppercase 'If' after the colon. The codebase is mixed on this convention — consider aligning with team preference.

• [comment-style] iib/workers/tasks/fbc_utils.py:176 — The inline comment says 'empty yaml object' — 'YAML' should be uppercase to match the codebase convention.

• [test-docstring-convention] tests/test_workers/test_tasks/test_fbc_utils.py:328 — The docstring uses 'a IIBError' which should be 'an IIBError'.

• [scope-creep] iib/workers/tasks/fbc_utils.py:162 — The PR adds .yml extension support which is not mentioned in the PR title or body. Consider documenting this in the PR description.

• [architectural-alignment] iib/workers/tasks/fbc_utils.py:176 — Empty YAML documents are silently skipped without logging. Consider adding log.debug(f"Skipping empty YAML document in {in_file}") for debuggability.

• [incomplete-documentation] iib/workers/tasks/fbc_utils.py:150 — The :raises IIBError: docstring could be more specific about conditions (0-byte YAML files).

• [incomplete-documentation] iib/workers/tasks/fbc_utils.py:154 — The docstring could explicitly mention support for both .yaml and .yml extensions.

• [incomplete-documentation] iib/workers/tasks/fbc_utils.py:154 — The docstring does not document the new behavior of skipping empty YAML objects during conversion.

Previous run

Review

Findings

High

• [error-handling-gap] iib/workers/tasks/fbc_utils.py:165 — The PR raises ValidationError for empty YAML files, but the Celery worker error callback in iib/workers/tasks/general.py:failed_request_callback only handles IIBError and FinalStateOverwriteError. Since ValidationError is a sibling class (both inherit from BaseException, not from each other), this exception will fall into the generic else branch, producing the message "An unknown error occurred. See logs for details" instead of the specific "Empty YAML file found: ..." message. Every other error raised in the worker task layer uses IIBError.
  Remediation: Raise IIBError instead of ValidationError for the empty YAML file check, consistent with all other error paths in the worker tasks. Alternatively, add ValidationError handling to failed_request_callback.

Medium

• [logic-error] iib/workers/tasks/fbc_utils.py:175 — When a YAML file contains only empty documents (e.g., just ---\n---\n), every chunk will be None and the continue will skip all of them. The output file is created (opened in append mode) as a 0-byte file, and then the original YAML file is removed. This leaves a 0-byte JSON file in the config directory, which will likely fail opm validate downstream. The 0-byte size check does not catch this case because the file has non-zero size.
  Remediation: After the for chunk in data loop, check whether anything was actually written to json_out. If nothing was written, either raise an error or delete the empty output file.

Low

• [style-convention] iib/workers/tasks/fbc_utils.py:175 — Inline comment # Ignore if it is an empty yaml object uses lowercase first letter and lowercase "yaml". The codebase predominantly capitalizes comment starts and uses uppercase "YAML" for the format name.
  Remediation: Change to # Skip empty YAML objects or similar.

• [edge-case] iib/workers/tasks/fbc_utils.py:163 — Minor TOCTOU gap between os.path.getsize(in_file) and the subsequent open(in_file, 'r'). Practically irrelevant for local worker file processing but noted for completeness.

Previous run (2)

Review

Findings

High

• [logic-error] iib/workers/tasks/fbc_utils.py:164 — The diff introduces ValidationError for the empty-file check, but ValidationError is never caught anywhere in the iib/workers package. All worker-side error handling catches IIBError (e.g., retry_if_exception_type(IIBError), except IIBError). The ValidationError handler exists only in the web layer (iib/web/errors.py and iib/web/app.py). When enforce_json_config_dir is called from merge_catalogs_dirs (invoked by handle_add_request and handle_rm_request in build.py), a ValidationError will propagate uncaught through the Celery task because the request_logger wrapper has no except clause (only try/finally). This will cause the Celery worker task to fail with an unhandled exception rather than a properly reported error.
  Remediation: Use IIBError instead of ValidationError for the empty-YAML-file check, consistent with all other error handling in the workers package.

Medium

• [stale-docstring] iib/workers/tasks/fbc_utils.py:151 — The docstring for enforce_json_config_dir does not document the new behavior: (1) handles both .yaml and .yml extensions, (2) raises an exception for empty files, (3) skips empty YAML documents. The current docstring only states "It will walk recursively and convert any YAML files to the JSON format." Since Sphinx automodule uses this as the primary API documentation, this is a meaningful gap.
  Remediation: Update the docstring to document .yml support, empty file validation, and empty document skipping.

Low

• [edge-case] iib/workers/tasks/fbc_utils.py:162 — The new .yml extension handling means if a directory contains both foo.yaml and foo.yml, both would write to foo.json, with the second silently overwriting the first. Unlikely in practice (FBC catalogs are tool-generated) but worth noting.

• [test-adequacy] tests/test_workers/test_tasks/test_fbc_utils.py:303 — No test covers the newly added .yml extension handling. Both existing and new tests use .yaml files only.

• [scope-expansion] iib/workers/tasks/fbc_utils.py:162 — The PR adds .yml extension support which is not mentioned in the PR description. The PR body only describes skipping empty YAML documents.

Previous run (3)

Review

Findings

Low

• [edge-case] iib/workers/tasks/fbc_utils.py:167 — If a YAML file contains only empty documents (all chunks are None), the code will create a 0-byte JSON output file after removing the original YAML. Downstream consumers (e.g., opm validate) may not handle an empty file gracefully.

Info

• [test-adequacy] tests/test_workers/test_tasks/test_fbc_utils.py:303 — The new test covers the primary scenario (empty document between two non-empty documents). It does not cover the edge case of a YAML file containing only empty documents (all-None chunks), which would now produce a 0-byte JSON file.
• [variable-naming] tests/test_workers/test_tasks/test_fbc_utils.py:317 — Variable name input shadows Python built-in. This pattern is established in the codebase (see line 199), but renaming to input_file/output_file would improve clarity.
• [coherence-assessment] iib/workers/tasks/fbc_utils.py:172 — The implementation is architecturally coherent with the existing enforce_json_config_dir function. The change follows the established pattern of iterating through YAML chunks and is consistent with the function's error-handling approach.

[ashwgit]

@yashvardhannanavati PTAL

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:22 AM UTC · Completed 6:33 AM UTC
Commit: 45f7743 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:50 AM UTC · Completed 6:58 AM UTC
Commit: 45f7743 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:27 AM UTC · Completed 8:38 AM UTC
Commit: 45f7743 · View workflow run →