Skip to content

fix(dbt): resolve assessor bugs and test fixture issues#358

Merged
kami619 merged 2 commits intomainfrom
bugfix/dbt-assessor-followups
Mar 26, 2026
Merged

fix(dbt): resolve assessor bugs and test fixture issues#358
kami619 merged 2 commits intomainfrom
bugfix/dbt-assessor-followups

Conversation

@kami619
Copy link
Collaborator

@kami619 kami619 commented Mar 26, 2026

Summary

Fixes 4 follow-up issues from the dbt assessor PR (#357/#334): a placeholder detection bug, fragile YAML file matching, 22 broken test fixtures, and 55 unnecessary static files.

Also fixes a pre-existing issue where 22 of 48 dbt assessor tests were erroring in CI due to missing .git directories in fixture folders.

Fixes #353, fixes #354, fixes #355, fixes #356

Changes

Bug Fixes

  • Placeholder detection (dbt.py:307-324): Removed "description" from placeholder_texts set and switched from substring to exact match. Descriptions containing common words like "description" are no longer falsely rejected.
  • YAML file discovery (dbt.py:33-44): Simplified _find_yaml_files() to always search both .yml and .yaml explicitly, removing the fragile pattern.replace("yml", "yaml") logic.

Test Fixes

  • Fixture helper (test_assessors_dbt.py:30-55): Added _repo_from_fixture() that copies fixture content into tmp_path with a .git directory, fixing Repository validation. All 22 previously-erroring tests now pass.
  • Dynamic flat_structure (test_assessors_dbt.py:96-115): Replaced 55 static SQL fixture files with dynamic tmp_path generation.
  • Regression test (test_assessors_dbt.py:347): Added test_assess_description_word_not_flagged_as_placeholder for Bug: DbtModelDocumentationAssessor false-negative on descriptions containing 'description' #353.

Verification (no code change)

Test Plan

🤖 Generated with Claude Code

kami619 and others added 2 commits March 26, 2026 03:16
- Fix #353: Remove "description" from placeholder_texts and switch to
  exact match to prevent false negatives on legitimate descriptions
- Fix #355: Replace 55 static flat_structure fixture files with dynamic
  tmp_path generation; fix all fixture-based tests by copying fixture
  content into tmp_path with .git directory (Repository validation)
- Fix #356: Simplify _find_yaml_files to always search both .yml and
  .yaml extensions instead of fragile pattern.replace()
- Fix #354: Verified scoring engine correctly excludes not_applicable
  assessors from weight normalization (no code change needed)

Fixes #353, fixes #354, fixes #355, fixes #356

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 26, 2026

Warning

Rate limit exceeded

@kami619 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 42 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9de60c5f-d53e-4618-af2c-358c7ee97275

📥 Commits

Reviewing files that changed from the base of the PR and between f1f098c and 9059ba2.

📒 Files selected for processing (58)
  • src/agentready/assessors/dbt.py
  • tests/fixtures/dbt_projects/flat_structure/dbt_project.yml
  • tests/fixtures/dbt_projects/flat_structure/models/model_1.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_10.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_11.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_12.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_13.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_14.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_15.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_16.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_17.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_18.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_19.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_2.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_20.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_21.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_22.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_23.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_24.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_25.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_26.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_27.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_28.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_29.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_3.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_30.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_31.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_32.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_33.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_34.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_35.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_36.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_37.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_38.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_39.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_4.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_40.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_41.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_42.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_43.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_44.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_45.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_46.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_47.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_48.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_49.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_5.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_50.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_51.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_52.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_53.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_54.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_55.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_6.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_7.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_8.sql
  • tests/fixtures/dbt_projects/flat_structure/models/model_9.sql
  • tests/unit/test_assessors_dbt.py

Warning

.coderabbit.yaml has a parsing error

The CodeRabbit configuration file in this repository has a parsing error and default settings were used instead. Please fix the error(s) in the configuration file. You can initialize chat with CodeRabbit to get help with the configuration file.

💥 Parsing errors (1)
Validation error: String must contain at most 250 character(s) at "tone_instructions"
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/dbt-assessor-followups

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.

@github-actions
Copy link
Contributor

📈 Test Coverage Report

Branch Coverage
This PR 67.2%
Main 67.1%
Diff ✅ +0.1%

Coverage calculated from unit tests only

@github-actions
Copy link
Contributor

AgentReady Code Review — PR #358

fix(dbt): resolve assessor bugs and test fixture issues


Overall Assessment

This is a solid follow-up cleanup PR. The core bugs are real and the fixes are correct. The _repo_from_fixture helper is the right abstraction and eliminates both the .git validation errors and the bloated static fixture directory. No security issues. A few observations below.


Bug Fixes — src/agentready/assessors/dbt.py

_find_yaml_files — signature simplification (line 33)

Approved. Removing the pattern parameter is the right call — all call sites only ever passed *.yml, and the old pattern.replace("yml", "yaml") logic was subtle and error-prone (e.g. a pattern like *schema.yaml would have produced *schema.yaaa). The new implementation is unambiguous.

Minor note: the function now returns all .yml/.yaml files, not just schema files. This was already the behavior in practice (the callers were looking at models/ so any non-schema YAML there would have been picked up too), but the doc comment could be more explicit that this is intentional. Not blocking.

Placeholder detection — dbt.py:309-325

Approved with a note. The removal of "description" from placeholder_texts is correct and directly fixes #353. However, the switch from substring matching to exact matching is a broader behavior change that isn't called out in the PR description:

  • Before: "todo: fill this in" → rejected (substring match on "todo")
  • After: "todo: fill this in"accepted (no exact match)

Only a description that is exactly "todo", "tbd", "fixme", or "placeholder" (after .strip().lower()) is now rejected.

This is more permissive than before. It will eliminate false positives, but it also weakens detection of lazy placeholder-style descriptions. If this is intentional policy, it should be documented. If not, an alternative fix is to keep substring matching but remove only "description" from the set — that would fix #353 without loosening detection for the others.


Test Changes — tests/unit/test_assessors_dbt.py

_repo_from_fixture helper (lines 26–51)

Approved. Clean design — copy fixture content to tmp_path, inject .git, return Repository. The **kwargs pattern correctly handles per-fixture metadata overrides without hard-coding defaults everywhere. The exist_ok=True on .git mkdir is correct defensive practice.

One edge case: if a fixture directory is empty, src.iterdir() returns nothing and the function silently produces a tmp_path with only .git. This is harmless but could give confusing failures. A guard isn't strictly needed, just worth being aware of.

flat_structure_repo fixture (lines 74–113)

Approved. Replacing 55 static files with tmp_path generation is the right move — it removes fixture bloat, makes the test intent clearer, and avoids the .git problem entirely. One small inconsistency: this fixture creates .git directly with (tmp_path / ".git").mkdir() (no exist_ok=True) rather than going through _repo_from_fixture. Not a bug since tmp_path is always fresh, but worth aligning for consistency.

Regression test test_assess_description_word_not_flagged_as_placeholder (lines 344–380)

Approved. Good specific regression test for #353. The inline repo construction is fine here — this is a targeted edge case that doesn't need a reusable fixture. The assertion score == 100.0 and status == "pass" cleanly validates the fix.

test_find_yaml_files (line 142)

Approved. The call correctly updated to the new signature. The assertion len(yaml_files) >= 2 is slightly weaker now (finds all yaml in models/, not just *schema.yml) but still validates the core behavior.


Score Impact

The placeholder detection relaxation could improve scores for repos with descriptions like "todo: complete this" that were previously falsely penalized. In practice this is a correctness improvement, not a regression.


Summary

Area Status
Bug fix: YAML discovery (#356) Approved
Bug fix: placeholder false positive (#353) Approved with note on exact-match scope
Test fixture .git errors (#354, #355) Approved
Static fixture cleanup Approved
Regression test coverage Approved
Security No issues

Recommendation: Approve — consider either documenting the exact-match placeholder behavior as intentional policy or filing a follow-up to revisit whether partial-phrase placeholders ("todo: ...") should still be caught.

🤖 Generated with Claude Code

@kami619 kami619 merged commit ec2e3de into main Mar 26, 2026
11 checks passed
@kami619 kami619 deleted the bugfix/dbt-assessor-followups branch March 26, 2026 03:32
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.31.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment