Skip to content

Conversation

@abgox
Copy link

@abgox abgox commented Oct 30, 2025

Description

Motivation and Context

Closes #6533

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Summary by CodeRabbit

  • New Features
    • Added a "GitHub" option for hash extraction. Configurations using this mode are now recognized and accepted, improving compatibility and reducing validation failures. No other public behavior or exported declarations were changed.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

Adds "github" to the hashExtraction.mode enum in the JSON schema and records the change in the changelog so manifests using github mode validate.

Changes

Cohort / File(s) Summary
Schema configuration
schema.json
Added "github" to definitions.hashExtraction.properties.mode.enum so hashExtraction.mode accepts github.
Changelog
CHANGELOG.md
Added an Unreleased entry documenting the addition of "github" to the schema's hash mode.

Sequence Diagram(s)

(No sequence diagram: change is a schema enum addition and does not affect runtime control flow.)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify schema.json JSON validity and correct placement of the new enum value.
  • Confirm CHANGELOG.md entry follows repository conventions.

Poem

🐰 I hopped through JSON, bright and spry,
Found a mode that made builds sigh.
I added "github" with a cheerful tap,
Now manifests pass — no more gap.
Hop, patch, and flourish — schema clap!

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 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(schema): add github to the mode of hash" is directly aligned with the main changes in the pull request. The changes add "github" as a new enum value to the hash.mode field in schema.json and include an accompanying CHANGELOG entry. The title is concise, clear, and uses conventional commit format, making it easy for reviewers scanning the history to understand that this is a schema fix addressing a missing mode option. The title accurately reflects the primary objective stated in the linked issue.
Linked Issues Check ✅ Passed The code changes directly satisfy the primary objective from linked issue #6533. The schema.json modification adds "github" to the mode enum under hashExtraction, which resolves the core problem that manifests using "github" for hash.mode were failing PR validation. The CHANGELOG.md entry documents this schema change, and the PR description confirms that all required tasks have been completed, including documentation and test updates. The changes precisely target the missing schema value that was causing validation failures, aligning with the issue's requirements.
Out of Scope Changes Check ✅ Passed The changes in this pull request are appropriately scoped to the linked issue. The modifications to schema.json add the required "github" value to the hash.mode enum, and the CHANGELOG.md entry documents this change. Both modifications directly support the stated objective of fixing the schema to prevent PR validation failures for manifests using github hash mode. No extraneous or unrelated changes are evident in the provided summary, and the PR body indicates that tests and documentation were updated to align with the fix, maintaining consistency within the scope of this issue.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 45e892b and 113ab0f.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

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.

Copy link

@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.

Actionable comments posted: 0

🧹 Nitpick comments (1)
CHANGELOG.md (1)

17-17: Minor grammar clarity improvement.

The phrase "to avoid pr validation to fail" could be slightly more natural. Consider: "to prevent PR validation failures" or "to prevent PR validation from failing".

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52a035b and 74cac61.

📒 Files selected for processing (2)
  • CHANGELOG.md (1 hunks)
  • schema.json (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T01:48:00.222Z
Learnt from: z-Fng
PR: ScoopInstaller/Scoop#6471
File: CHANGELOG.md:9-9
Timestamp: 2025-08-31T01:48:00.222Z
Learning: The Scoop project's CHANGELOG.md follows a convention of tracking PR numbers only, not issue numbers, according to the maintainer z-Fng.

Applied to files:

  • CHANGELOG.md
🔇 Additional comments (1)
schema.json (1)

51-62: LGTM!

The addition of "github" to the mode enum is minimal and correct. This allows manifests using GitHub hash extraction to pass schema validation.

@abgox
Copy link
Author

abgox commented Oct 30, 2025

  • @z-Fng
  • It will affect all pr validations.
  • I think it need a quick review.

  • By the way, it might also be necessary to set the scoop_branch of pr validation to develop.
  • Otherwise, as long as these fixes are not merged into master and the following content is used in the manifest, pr validation will definitely fail.
"hash": {
    "mode": "github"
}

@z-Fng
Copy link
Member

z-Fng commented Oct 30, 2025

Otherwise, as long as these fixes are not merged into master and the following content is used in the manifest, pr validation will definitely fail.

"hash": {
    "mode": "github"
}

The hash.mode field is rarely used in the manifests of the Main/Extras/Versions buckets.

It will affect all pr validations.

This can be easily resolved by removing the hash.mode field. It will automatically fall back to GitHub's predefined hash extraction.

I think it need a quick review.

Got it, noticed. It will be fixed in the next update.

@z-Fng
Copy link
Member

z-Fng commented Oct 31, 2025

And perhaps a code comment (or test) should be added to remind developers to synchronize the new mode to schema.json.

@abgox
Copy link
Author

abgox commented Oct 31, 2025

  • As you said, most buckets are not using this field now; only abyss is using it.
  • Maybe we can transition gradually.
  • After all, this is a field that is already supported in the core.

@z-Fng
Copy link
Member

z-Fng commented Oct 31, 2025

And perhaps a code comment (or test) should be added to remind developers to synchronize the new mode to schema.json.

Yeah, for now, adding a code comment in the get_hash_for_app function (autoupdate.ps1) should be fine.

@abgox
Copy link
Author

abgox commented Oct 31, 2025

  • By the way, the current schema is really very simple, even a bit crude.
  • It provides very little help in the actual work of writing manifest.
  • I think we can refer to scoop-manifest.* in abgox/schema, which provides more complete validation prompts.
  • Combined with abgox/json-schema-plus, it can also dynamically load Schema for different language environments.

@z-Fng
Copy link
Member

z-Fng commented Oct 31, 2025

Perhaps we can optimize it later. It's probably not part of the plan at the moment.

@z-Fng
Copy link
Member

z-Fng commented Nov 3, 2025

cc: @niheaven Can we merge this?

@niheaven niheaven merged commit 7966971 into ScoopInstaller:develop Nov 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants