Skip to content

fix(registry): don't fallback embeddedMetaJSON to default empty JSON#275

Open
yhp20170308-dotcom wants to merge 1 commit intolarksuite:mainfrom
yhp20170308-dotcom:fix/registry-embedded-meta-fallback
Open

fix(registry): don't fallback embeddedMetaJSON to default empty JSON#275
yhp20170308-dotcom wants to merge 1 commit intolarksuite:mainfrom
yhp20170308-dotcom:fix/registry-embedded-meta-fallback

Conversation

@yhp20170308-dotcom
Copy link
Copy Markdown

@yhp20170308-dotcom yhp20170308-dotcom commented Apr 6, 2026

Summary

  • loader_embedded.go was setting embeddedMetaJSON to the default empty JSON ({"version":"0.0.0","services":[]}) when meta_data.json is not compiled in
  • This caused hasEmbeddedData() to return true (34 bytes > 0), even though there were no real services
  • As a result, two tests failed instead of being correctly skipped:
    • TestColdStart_UsesEmbedded: failed with "expected embedded projects, got none"
    • TestCacheHit_WithinTTL: failed with "expected calendar from embedded data"

Fix

Remove the else fallback so embeddedMetaJSON remains nil when no real meta_data.json is present. hasEmbeddedData() then correctly returns false and dependent tests skip as intended.

Test plan

  • go build ./... — passes
  • go vet ./... — no warnings
  • TestColdStart_UsesEmbedded — now correctly SKIP (was FAIL)
  • TestCacheHit_WithinTTL — now PASS (was FAIL)
  • All other tests — still pass

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of missing embedded metadata. The system now properly detects when metadata is unavailable instead of applying default fallback values, enabling more accurate downstream behavior.

When meta_data.json is not compiled in, embeddedMetaJSON was being set
to meta_data_default.json ({"version":"0.0.0","services":[]}), which is
34 bytes. This caused hasEmbeddedData() to return true even though there
were no real services, making TestColdStart_UsesEmbedded fail instead of
skip, and TestCacheHit_WithinTTL fail on the embedded calendar check.

Fix: leave embeddedMetaJSON as nil when no real meta_data.json is present,
so hasEmbeddedData() correctly returns false and dependent tests skip.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


yanghongping seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added the size/M Single-domain feat or fix with limited business impact label Apr 6, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 6, 2026

📝 Walkthrough

Walkthrough

The init() function in the embedded registry loader was modified to stop falling back to default embedded metadata when the meta_data.json file is missing or unreadable. Instead, embeddedMetaJSON remains nil, allowing downstream code to detect the absence of embedded data and skip related tests.

Changes

Cohort / File(s) Summary
Embedded Metadata Fallback Removal
internal/registry/loader_embedded.go
Removed fallback to embeddedMetaDataDefaultJSON when meta_data.json read fails; embeddedMetaJSON now remains unset (nil) to enable proper absence detection in downstream checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 The fallback is gone, now we see clear,
When metadata's missing, we acknowledge it here!
No defaults to hide what simply ain't there,
Downstream checks smile—the nil's showing care! 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing the fallback to default empty JSON for embeddedMetaJSON.
Description check ✅ Passed The description covers all required sections with comprehensive details about the motivation, fix, and test plan verification.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
internal/registry/loader_embedded.go (1)

11-12: Remove the unused embeddedMetaDataDefaultJSON variable and its embed directive.

The variable is no longer referenced after removing the fallback branch. Deleting the //go:embed meta_data_default.json directive and variable declaration at lines 11-12 will reduce binary size and improve code clarity.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/registry/loader_embedded.go` around lines 11 - 12, Remove the unused
embed directive and variable by deleting the `//go:embed meta_data_default.json`
line and the `var embeddedMetaDataDefaultJSON []byte` declaration (symbol:
embeddedMetaDataDefaultJSON and its embed comment) from the file so the unused
embedded asset is not compiled into the binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@internal/registry/loader_embedded.go`:
- Around line 11-12: Remove the unused embed directive and variable by deleting
the `//go:embed meta_data_default.json` line and the `var
embeddedMetaDataDefaultJSON []byte` declaration (symbol:
embeddedMetaDataDefaultJSON and its embed comment) from the file so the unused
embedded asset is not compiled into the binary.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 49e540bd-03bd-4d9e-8e85-2d07a1728ebe

📥 Commits

Reviewing files that changed from the base of the PR and between 0c77c95 and c63357e.

📒 Files selected for processing (1)
  • internal/registry/loader_embedded.go

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 6, 2026

Greptile Summary

Removes the else branch in loader_embedded.go that was assigning the default empty JSON to embeddedMetaJSON, which caused hasEmbeddedData() to return true even when no real services were compiled in, breaking two dependent tests. The fix is correct and minimal.

As a minor follow-up, embeddedMetaDataDefaultJSON (lines 11–12) is now unreferenced — and since meta_data_default.json already matches the meta_data*.json glob used by metaFS, the file ends up embedded twice in the binary with no functional benefit.

Confidence Score: 5/5

Safe to merge; correctly prevents false-positive hasEmbeddedData() results with no functional regressions.

Single-line removal with a clear, well-described intent. The one remaining finding is P2 (unused dead variable / double-embed) and does not block merge.

No files require special attention; the unused embeddedMetaDataDefaultJSON declaration in loader_embedded.go is worth cleaning up as a follow-up.

Important Files Changed

Filename Overview
internal/registry/loader_embedded.go Removes else fallback that set embeddedMetaJSON to default empty JSON; embeddedMetaDataDefaultJSON is now unreferenced dead code that also double-embeds meta_data_default.json

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[init called] --> B{metaFS.ReadFile meta_data.json\nsucceeds AND len > 0?}
    B -- Yes --> C[embeddedMetaJSON = data]
    B -- No --> D[embeddedMetaJSON remains nil]
    C --> E[hasEmbeddedData returns true]
    D --> F[hasEmbeddedData returns false]
    F --> G[Tests needing embedded data: SKIP]
    E --> H[Tests needing embedded data: RUN]
Loading

Comments Outside Diff (1)

  1. internal/registry/loader_embedded.go, line 11-12 (link)

    P2 Dead embedded variable after removing else branch

    embeddedMetaDataDefaultJSON is declared with //go:embed meta_data_default.json but is now unreferenced anywhere in the codebase. The //go:embed directive still causes the file to be included in the binary at build time regardless of whether the variable is ever read. Since meta_data_default.json also matches the meta_data*.json glob used by metaFS on line 8, the file ends up embedded twice in the binary with no benefit. Consider removing this declaration as a follow-up cleanup.

Reviews (1): Last reviewed commit: "fix(registry): don't fallback embeddedMe..." | Re-trigger Greptile

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

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants