Skip to content

fix: custom w3c document loader#323

Merged
GHkrishna merged 1 commit intomainfrom
fix/custom-document-loader
Oct 17, 2025
Merged

fix: custom w3c document loader#323
GHkrishna merged 1 commit intomainfrom
fix/custom-document-loader

Conversation

@GHkrishna
Copy link
Copy Markdown
Contributor

@GHkrishna GHkrishna commented Oct 17, 2025

What:

Fix incorrect custom document loader injection while creating agent

Summary by CodeRabbit

  • Bug Fixes
    • Fixed the initialization of the credentials module to properly respect the custom document loader configuration setting.

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
@GHkrishna GHkrishna requested a review from sairanjit October 17, 2025 12:42
@GHkrishna GHkrishna self-assigned this Oct 17, 2025
@GHkrishna GHkrishna added the bug Something isn't working label Oct 17, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 17, 2025

Walkthrough

The W3cCredentialsModule instantiation in src/cliAgent.ts has been inverted: when isCustomDocumentLoaderEnabled() returns true, a documentLoader is now supplied; when false, it is omitted. This reverses the previous conditional logic for module wiring.

Changes

Cohort / File(s) Summary
W3cCredentialsModule instantiation logic
src/cliAgent.ts
Inverted ternary condition for W3cCredentialsModule construction: documentLoader is now supplied when isCustomDocumentLoaderEnabled() is true and omitted when false, reversing the prior logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • credebl/credo-controller#321: Introduces isCustomDocumentLoaderEnabled() and CustomDocumentLoader, directly enabling the conditional logic now applied in this PR's module construction inversion.

Suggested reviewers

  • RinkalBhojani
  • sairanjit

Poem

🐰 A flip of the switch, a twist of the flow,
Where true means inclusion, and false lets it go!
The module now listens to what we command,
Custom documents loaded, just as we'd planned! 📋✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "fix: custom w3c document loader" directly addresses the main change in the changeset. The summary indicates the PR fixes incorrect custom document loader injection logic in the W3cCredentialsModule construction, specifically correcting a reversed ternary condition in src/cliAgent.ts. The title is concise, specific, and clearly conveys that this is a bug fix related to custom W3C document loader functionality, which aligns perfectly with the PR's objective to fix incorrect custom document loader injection while creating an agent.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/custom-document-loader

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dfefc8e and e9d08ab.

📒 Files selected for processing (1)
  • src/cliAgent.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cliAgent.ts (1)
src/utils/customDocumentLoader.ts (1)
  • CustomDocumentLoader (28-47)
🔇 Additional comments (2)
src/cliAgent.ts (2)

188-192: Logic inversion bug fixed correctly.

The conditional now properly aligns with the semantic meaning of isCustomDocumentLoaderEnabled():

  • When true → custom document loader is injected ✓
  • When false → default behavior (no custom loader) ✓

This is a critical fix that reverses the previous inverted logic where enabled/disabled states were producing opposite behaviors.


188-192: Fix verified as logically correct, but integration tests do not exist in this repository for manual verification.

The conditional logic is correct: when isCustomDocumentLoaderEnabled() returns true, the W3cCredentialsModule is instantiated with the CustomDocumentLoader; when false, it uses the default. The implementation matches the intended behavior.

However, no integration or unit tests were found in the codebase to verify this fix at runtime. To ensure the fix works correctly in production:

  1. Either add integration tests covering:

    • Custom document loader activates when ENABLE_CUSTOM_DOCUMENT_LOADER=true and required environment variables are set
    • Default behavior applies when the flag is false
    • URL replacement for deprecated W3C domains works as expected
  2. Or manually verify the behavior in a test environment by:

    • Setting ENABLE_CUSTOM_DOCUMENT_LOADER=true with valid DEPRECATED_DOMAIN and CURRENT_DOMAIN values
    • Confirming requests to deprecated domains are intercepted and resolved correctly
    • Verifying default behavior when the flag is disabled

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@sairanjit sairanjit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GHkrishna GHkrishna merged commit bf5fa4b into main Oct 17, 2025
9 checks passed
@GHkrishna GHkrishna deleted the fix/custom-document-loader branch October 17, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants