Improve cross-platform config handling and normalize config update behavior

[ANSHIKATYAGI30]

Related Issue

Closes #370

Proposed Changes

• Modified plugins.py
• Changes made-:

1. Added cross-platform config path support
2. Normalized dryRun handling
3. Improved config merge behavior
4. Improved YAML safety handling

Type of Change

• 🐛 Bug fix
• ✨ New feature
• 📖 Documentation
• 🧪 Testing
• 🛠️ Refactoring
• 🚀 DevOps/CI

Testing & Verification

• I have run dotnet test and all 60+ cross-platform tests passed.
• I have verified the changes on a Windows environment (if applicable).
• I have added new unit tests to cover my changes.

GSSOC 2026 Checklist

• I have read the Contribution Guidelines.
• My code is formatted correctly (I have run dotnet format, uv tool run ruff format plugins/, and bun x prettier --write plugins/).
• I have linked the PR to an approved issue.
• I understand that a maintainer must apply the gssoc:approved label for this PR to count for points.

[ANSHIKATYAGI30]

@DotDev262 Kindly review the PR.
Thanks.

[DotDev262]

6 issues to fix

Thanks for the PR! This is assigned to the right issue (#370). However, there are several protocol violations that need to be fixed:

1. check_installed must return bare bool
Currently returns {"data": bool}. check_installed() must return a bare bool, and main() should wrap it:

if command == "check_installed":
    installed = check_installed(args, request_id)
    response = {"requestId": request_id, "installed": installed}

2. Empty stdin must return a JSON error
if not raw: return exits silently with no stdout — this hangs the host. Must return a JSON error response:

if not raw:
    sys.stdout.write(json.dumps({"requestId": "unknown", "error": "No input received"}) + "\n")
    return

3. dryRun must come from args, not context
dry_run = bool(context.get("dryRun", False)) should be dry_run = args.get("dryRun", False).

4. Non-standard response fields
Responses include "success", "changed", and "data" fields that are not part of the standard protocol. Stick to "requestId", "installed"/"changed", and "error".

5. requestId pattern
request.get("requestId", "unknown") returns None when key exists with null value. Use request.get("requestId") or "unknown".

6. Error responses include extra fields
The exception handler returns "success": False and "changed": False alongside "error". Error responses should only contain "requestId" and "error".

Please fix and push — happy to re-review.

[ANSHIKATYAGI30]

@DotDev262 please review

[DotDev262]

Critical bug in main() — breaks all non-empty input

Good progress on most fixes, but the updated main() has a critical bug:

def main() -> None:
    raw = sys.stdin.read()
    if not raw:
        sys.stdout.write(
            json.dumps({"requestId": "unknown", "error": "No input received"}) + "\n"
        )
    sys.stdout.flush()
    return  # ← This runs for ALL input, not just empty!

sys.stdout.flush() and return are outside the if not raw: block. The function flushes stdout and returns immediately for every call — valid input is never processed.

Fix:

def main() -> None:
    raw = sys.stdin.read()
    if not raw:
        sys.stdout.write(
            json.dumps({"requestId": "unknown", "error": "No input received"}) + "\n"
        )
        sys.stdout.flush()
        return

    try:
        request = json.loads(raw)
    ...

Indent the sys.stdout.flush() and return inside the if block.

[DotDev262]

@ANSHIKATYAGI30 — please rebase on latest main (3 commits behind) so I can re-review your latest fixes.

[ANSHIKATYAGI30]

@DotDev262 please review once again!
Thank you..

[DotDev262]

Good progress on the cross-platform config path and protocol fixes. However, I found a critical regression: dry-run mode is broken.

🚫 Dry-run no longer works

What changed:

• Old handle() passed the full args (including dryRun) to apply_config()
• New handle() extracts settings = args.get("settings", {}) and passes only settings
• But the new apply_config() reads dryRun from args — which is now the settings dict

Result: dryRun is always False regardless of what the caller sets. Any request with "dryRun": true will silently apply changes.

Fix suggested:

In handle():

if command == "apply":
    if not isinstance(args, dict):
        return {"requestId": request_id, "error": "args must be a dictionary"}
    return apply_config(request_id, args)

In apply_config():

def apply_config(request_id: str, args: dict) -> dict:
    dry_run = bool(args.get("dryRun", False))
    settings = args.get("settings", {})
    if not isinstance(settings, dict):
        return {"requestId": request_id, "error": "settings must be a dictionary"}
    updates = {k: v for k, v in settings.items()}
    ...

This keeps the full args available so dryRun is accessible, while still properly extracting settings.

Please also rebase onto latest main (currently 0 behind, good) and re-request review.

[ANSHIKATYAGI30]

@DotDev262 Thank you so much for explaining everything so clearly. I've made the necessary changes. Please review once again.

[DotDev262]

10 remaining issues to fix

In src/plugin.py:

1. "success" and "changed" in exception handler (lines 165-166) — these fields are banned. Error responses should only contain requestId and error:

   "requestId": request.get("requestId") or "unknown" if "request" in locals() and isinstance(request, dict) else "unknown",
   "error": str(error),

2. handle() raises ValueError (line 146) — should return an error dict instead:

   return {"requestId": request_id, "error": f"Unknown command: {command}"}

3. write_yaml not atomic (lines 51-57) — writes directly to file. Must use tempfile.mkstemp() + os.replace().

4. value == None (line 64) — should be value is None.

In test/test_gh.py:

5. Tests check response["success"] — 6 tests fail because the plugin no longer returns "success". Remove these assertions.

6. Tests pass dryRun in context (lines 77, 128, 151, 171) — the plugin reads dryRun from args now, so dry-run configs are never applied. Move "dryRun": true into args.

7. Tests check response["data"] (lines 40, 49) — "data" wrapper is banned. For check_installed, the response is {"requestId": ..., "installed": ...}.

8. Test passes "dry_run" (snake_case) (line 150) — the plugin expects "dryRun" (camelCase) in args.

9. sys.path.insert(0) (line 18) — use sys.path.append + sys.path.remove or importlib instead.

10. 2 commits behind main — please rebase onto latest main.

Please fix and re-request review.

[ANSHIKATYAGI30]

@DotDev262 please check!

[DotDev262]

@ANSHIKATYAGI30 This PR has been through 4 rounds of review with the atomic write fix still outstanding. Please respond within 24 hours with an update or this PR will be closed and the issue will be reassigned.

[DotDev262]

Critical: Atomic write required

The write_yaml function still uses a direct write pattern that can produce partially-written files if the process crashes mid-write:

with open(file_path, "w", encoding="utf-8") as file_handle:
    yaml.dump(data, file_handle, default_flow_style=False)

This must use atomic writes to prevent corruption — per project convention. Fix:

fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".yml")
try:
    with os.fdopen(fd, "w", encoding="utf-8") as f:
        yaml.dump(data, f, default_flow_style=False)
    os.replace(tmp_path, file_path)
except Exception:
    os.unlink(tmp_path)
    raise
``"

Everything else looks good — protocol-compliant, cross-platform support, no more `"success"` fields, tests assert `changed`. Just this one fix needed.

[ANSHIKATYAGI30]

@DotDev262 please check!

[DotDev262]

@ANSHIKATYAGI30 Please rebase onto latest main. Let me know when done.

[ANSHIKATYAGI30]

@DotDev262 done!.. please review!

[DotDev262]

Thanks for addressing the cross-platform config issues! I've reviewed the changes and can see:\n\n✅ write_yaml is now atomic: Uses tempfile.mkstemp + os.replace (was using open(file_path, "w"))\n\n❗ Issues remaining to address:\n\n1. Tests don't assert changed field: None of the test methods in test_gh.py assert the "changed" field in the response. They check config content and other side effects but don't verify that the plugin correctly returns changed:true/false in the response JSON. This needs to be fixed.\n\n2. Lint failures: The lint check is failing. Please run and 13 files reformatted, 93 files left unchanged to fix formatting issues.\n\nOnce you've added assertions for the changed field in the test cases and fixed the lint failures, this should be ready for approval. The cross-platform path handling looks correct!

[ANSHIKATYAGI30]

@DotDev262 Made the necessary changes. please check!

[DotDev262]

Most protocol issues are fixed — thanks. One issue remaining:

os.replace inside with os.fdopen block: The replace must happen after the file descriptor is closed, otherwise the content may not be fully flushed to disk. Move os.replace outside the with block:

Also, sort_keys=False was dropped from yaml.dump — was that intentional? It changes the output key ordering.

[ANSHIKATYAGI30]

@DotDev262
Thanks for catching that. I've updated the atomic write implementation so that os.replace() is executed only after the temporary file handle is closed and I've also restored sort_keys=False in yaml.dump() to preserve the existing key ordering behavior and avoid introducing unnecessary config diffs. please check!

[DotDev262]

@ANSHIKATYAGI30 the gh plugin failed its test , please check it as well as before committing run a ruff check since the linter is failing as well on the gh plugin

[DotDev262]

Hi @ANSHIKATYAGI30! Just a friendly reminder — this PR is 20 commits behind main and CI is still failing (gh test + lint). Could you please rebase and check on the failures? Thanks!

[DotDev262]

Test assertions don't match response format

✅ Plugin code is fully protocol-compliant:

• check_installed returns bare bool
• main()/handle() wraps responses correctly
• dryRun from args
• requestId uses .get() or "unknown"
• No success/data fields
• Empty stdin returns JSON error
• Atomic writes via mkstemp + os.replace ✅
• settings from args.get("settings", {}) with isinstance guard

❌ 4 tests fail because they assert response["changed"] on non-apply responses:

The plugin's check_installed returns {"requestId": ..., "installed": ...} (no "changed" field), and error responses return {"requestId": ..., "error": ...}. But these tests still check response["changed"]:

1. test_check_installed_returns_true_when_gh_is_found (line 38): remove self.assertFalse(response["changed"])
2. test_check_installed_returns_false_when_gh_is_missing (line 46): remove self.assertFalse(response["changed"])
3. test_apply_returns_error_when_pyyaml_is_missing (line 183): remove self.assertFalse(response["changed"])
4. test_unknown_command_returns_error (line 196): remove self.assertFalse(response["changed"])

Please remove these 4 stale assertions and re-push. The apply tests (lines 80, 129, 150, 168) correctly assert changed and will continue to pass.

[DotDev262]

Your branch is behind main. Please rebase onto latest main and force-push so we can proceed with the review. Thanks!