feat: add topgrade configuration management plugin#369
Conversation
|
Hi maintainers, just a heads-up the You can verify by running: git checkout main
uv tool run ruff check plugins/everything/src/plugin.py |
DotDev262
left a comment
There was a problem hiding this comment.
Thanks for the PR. Several issues to fix:
1. CRITICAL: Empty stdin causes silent exit (host hangs)
if not raw_input: return returns nothing to stdout. Must return a JSON error response:
{"requestId": "unknown", "error": "No input received"}2. CRITICAL: JSON parse error has no stdout response
Writing to stderr only and returning silently still leaves the host hanging. Must print a JSON error to stdout.
3. check_installed must return bare bool
Returns {"data": {"installed": bool}}. Must return bare bool, and main() wraps it:
result = handle_check_installed()
response = {"requestId": req_id, "installed": result}4. dryRun must come from args, not context
dry_run = context.get("dryRun", False) should be args.get("dryRun", False).
5. requestId must use .get("requestId") or "unknown"
request.get("requestId") returns None when key exists but is null.
6. Non-standard response fields
"success" and "changed" are not standard. Use only requestId, error, and action-specific fields.
7. response.update() merge pattern
The response.update(res) pattern breaks when functions return non-dict values. Design each handler to return data, and let main() construct the response.
Please reference existing merged plugins (e.g., plugins/pnpm/) for correct patterns.
|
@AdityaM-IITH — Thanks for addressing the review feedback! The PR needs a rebase before I can do a final review. Please rebase onto latest main. |
6547f4f to
636c48f
Compare
|
@AdityaM-IITH — please rebase on latest main (6 commits behind) so I can re-review. Let me know if you need help. |
636c48f to
aae085f
Compare
|
@AdityaM-IITH Please rebase onto latest main before review. Let me know when done. |
|
@AdityaM-IITH Please rebase onto latest main. Let me know when done. |
aae085f to
7ea1250
Compare
|
@DotDev262 Rebased onto latest main. I also fixed the pre-existing linting errors that were failing CI, so everything should pass cleanly now. |
DotDev262
left a comment
There was a problem hiding this comment.
Hi @AdityaM-IITH,
Thank you for rebasing this PR and updating the format!
However, the plugin does not yet comply with the repository's updated JSON-RPC protocol contract and guidelines. Please address the following issues:
-
check_installedReturn Type:- Per the repository guidelines,
check_installedmust return a barebool. It is the responsibility ofmain()to wrap it into the standard JSON response format:{"requestId": request_id, "installed": result}. - Currently,
check_installedreturns a dictionary includingsuccessanddatafields.
- Per the repository guidelines,
-
Banned Response Fields (
successanddata):- The
"success"field and"data"wrapper are banned from all JSON-RPC responses. - Currently, they are used across all responses in
check_installed,apply_config, and the error handlers. Please remove them and return flat response objects containing only the required fields (e.g.requestId,changed,installed,error).
- The
-
Non-Atomic File Writes:
- In
apply_config(lines 113–114), the file is written to directly usingopen(config_path, "w"). - The repository requires atomic writes using
tempfile.mkstemp()andos.replace()to prevent configuration corruption.
- In
-
Test Refactoring:
- Once the response schema is updated to remove
successanddataand wrapcheck_installedproperly, the unit tests inplugins/topgrade/test/test_topgrade.pymust be updated to match the new schema (i.e. assert oninstalleddirectly and removesuccess/dataassertions).
- Once the response schema is updated to remove
-
Rebase Required:
- The branch is 1 commit behind
maindue to a recent merge. Please rebase your branch on the latestmain.
- The branch is 1 commit behind
Thank you!
|
Hi @AdityaM-IITH! Just a friendly reminder — this PR is 10 commits behind main and has outstanding protocol issues (check_installed bare bool, success/data fields). Could you please rebase and address the remaining feedback? Thanks! |
DotDev262
left a comment
There was a problem hiding this comment.
Your branch is behind main. Please rebase onto latest main and force-push so we can proceed with the review. Thanks!
…rom args, remove response.update pattern
7e0aa2e to
c8f9726
Compare
|
@DotDev262 Rebase is complete. I've also fully addressed the protocol issues: check_installed now returns a bare bool, which main() handles. |
Related Issue
Closes #186
Proposed Changes
topgradesystem-wide upgrade configuration (topgrade.toml).config_providercapability usingtomlkitto safely merge incoming settings while perfectly preserving existing user comments and TOML formatting.uvautomatically handles dependencies without manual pip installs.plugins/topgrade/plugin.yamlmanifest.plugins/topgrade/src/plugin.pycontaining the core config parsing and merging logic.plugins/topgrade/test/test_topgrade.pyfor automated tests of the new plugin.Type of Change
Screenshots / Logs (if applicable)
Testing & Verification
dotnet testand all 60+ cross-platform tests passed.GSSOC 2026 Checklist
dotnet format,uv tool run ruff format plugins/, andbun x prettier --write plugins/).gssoc:approvedlabel for this PR to count for points.