Git import: null project descriptions scan as blank instead of failing - #1752
chiang-daniel wants to merge 1 commit into
Conversation
Git Auto Sync scans a whole repo for project.kiln files and validates them
as a single list, so one bad file takes down the entire scan. A project file
carries an explicit null for any field that was never set, but the scan read
it with data.get("description", "") -- a default that only covers a missing
key, not a null one -- and ProjectInfo.description was a required
non-nullable str.
One null description therefore made every project in the repo unimportable,
with an error that named no file, no project and no step.
These fields are display-only and come from files we did not write, so a
null now reads as blank: the scan coerces, and ProjectInfo no longer rejects
one. id had the same latent exposure and is covered too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R9sYts5tsdPzNstbhbg3F8
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughThe project scanner and ChangesProject metadata normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Project imports now tolerate unset name, description, and ID fields by treating them as blank, allowing valid projects in the same repository to remain available. The covered behavior is ready to merge. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 4 files. (1 skipped: 1 too large.) Full details: Description checkExplanation The description clearly explains the problem, cause, scope, implementation, tests, and checklist status. It omits the required Related Issues and Contributor License Agreement sections, but the core description is complete.
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEAD
Summary
|
chiang-daniel
changed the title
chiang-daniel
changed the title
1 participant
What does this PR do?
Treats a project's name, description and id as blank when they arrive as null, so one project file can't block importing the rest of the repo.
Why
Project.descriptionisstr | None— null is a legal value, and it's what gets written for a description that was never set. The import scan assumed otherwise. It reads project files as raw JSON rather than loading them asProject, pulled the value withdata.get("description", "")— a default that only covers a missing key, not a null one — and handed the result toProjectInfo.description, typed as a required non-nullablestr.So two contracts disagreed, and a file that was valid to write was invalid to read. Since the scan validates everything it finds as a single list, one null made every project in the repo unimportable — with an error ("Description: Input should be a valid string") naming no file, no project and no step. Found on a repo with 13 projects, 12 of them fine.
To be clear about what this is not: we aren't relaxing validation to accept broken data. The file was always valid.
ProjectInfowas simply typed more strictly than the datamodel it describes, and this brings the reader back in line with the contract that already exists.The win
These fields are display-only and come from files we didn't write, so a null should read as blank rather than hide twelve valid projects next to it. Fixed at both layers: the scan coerces, and
ProjectInfono longer rejects a null.idhad the same latent exposure and is covered too.Nothing changes on the write side. Null stays legal, because every project file already on disk with one has to keep loading.
Checklists
🤖 Generated with Claude Code