Skip to content

refactor: remove AutoApproveConfig.load/.save and route all I/O through SyftBgConfig#9437

Open
pjwerneck wants to merge 12 commits into
devfrom
pjwerneck/fix-auto-approve-config
Open

refactor: remove AutoApproveConfig.load/.save and route all I/O through SyftBgConfig#9437
pjwerneck wants to merge 12 commits into
devfrom
pjwerneck/fix-auto-approve-config

Conversation

@pjwerneck

Copy link
Copy Markdown
Collaborator

Summary

Remove AutoApproveConfig.load()/.save(); all config I/O now goes through SyftBgConfig (CLI commands, api.py, orchestrator.py, JobApprovalHandler, save_gcp_project_id).

Changes

  • Add SyftBgConfig.edit(), a locked (fcntl) read-modify-write context manager; skips save() if the block raises.
  • Fix config correctness bugs: unknown top-level YAML keys were being dropped on save (extra="allow"); merged common fields (do_email, token paths) were getting permanently baked into sub-configs on every save, defeating inheritance (merge=False for edit()); per-service token-path overrides were getting clobbered by the parent's default.
  • Fix auto_approve(): copy/hash files into a private staging directory first, resolve naming and final placement atomically under one lock — eliminates a filesystem race where concurrent callers with the same auto-generated name could corrupt or orphan each other's entries, and handles a stale/orphaned target directory as a clean failure instead of an unhandled OSError.
  • Extract a shared file_lock() helper (common/locking.py); dedupes the identical lock logic that used to live separately in SyftBgConfig.edit() and JsonStateManager._file_lock.
  • init() now derives both gmail_token_path and drive_token_path from a single OAuth token path and returns the canonical (post-copy) path.
  • Widen auto_approve()/resolve_content_files() to Sequence[str | Path] (was list[str | Path], which rejected valid list[str]/list[Path] callers due to list invariance).
  • Update notebooks/ai_audit/internal/test_email_e2e.ipynb off the removed AutoApproveConfig.load() API.
  • Fix a test-isolation bug in test_email_auto_approve_flow.py that was silently leaking auto-approval directories into the real ~/.syft-bg instead of a tmp dir.

Testing

  • Added 19 new test cases.
  • Added `tests/unit/syft_bg/test_init.py
  • Added regression tests tests/unit/syft_bg/test_ensure_running.py
  • Existing tests modified to the new SyftBgConfig-based code.

Asana task

https://app.asana.com/1/1185126988600652/project/1210542925864934/task/1214797316252530

pjwerneck added 11 commits July 7, 2026 14:23
- Prevent persisting defaults from subservices into config.
- Save values not in the model (backwards compatibility with AutoApproveConfig)
- Prevent remove_auto_approve not-found path from persisting empty config.yaml on a no-op failure.
- Use a temp dir and perform all name resolution and directory placement
  atomically under a single edit() lock.
- Handle OSError on missing directory  when renaming
- Extract locking logic to common.locking file
- Fix unpatched test case writing to real ~/.syft-bg directory.
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors syft-bg configuration handling by removing AutoApproveConfig.load()/.save() and routing config I/O through SyftBgConfig, adding a shared file-lock helper to make read-modify-write cycles safe and improving YAML merge/save correctness. It also hardens auto_approve() against filesystem races by staging file copies and resolving final placement atomically under a lock, with accompanying test updates and new regression coverage.

Changes:

  • Centralize config persistence in SyftBgConfig (load(), edit()), preserve unknown top-level YAML keys, and avoid baking inherited common fields into service sub-configs on save.
  • Introduce common/locking.py:file_lock() and reuse it in both config editing and JSON state writes.
  • Update auto-approval flow (auto_approve()/CLI/tests/notebook) to use SyftBgConfig and improve concurrency-safety around auto-approval directory creation.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/test_job_auto_approval.py Update tests to read auto-approve config via SyftBgConfig.load().approve and patch default-path providers consistently.
tests/unit/syft_bg/test_init.py New unit tests for init token handling and canonical token-path behavior.
tests/unit/syft_bg/test_ensure_running.py New regression tests to ensure locked edits persist settings and missing config doesn’t create files.
tests/unit/syft_bg/test_email_auto_approve_flow.py Update email auto-approve flow tests to use SyftBgConfig and patched default paths.
tests/unit/syft_bg/test_criteria.py Update handler-config setup to write SyftBgConfig with embedded approve config.
tests/unit/syft_bg/test_config.py Extend config tests to cover SyftBgConfig.edit(), unknown-key preservation, and merge/save regressions.
tests/unit/syft_bg/test_auto_approve_api.py Add/extend tests for locking scope, name collisions, orphan-dir failure, and updated config routing.
tests/integration/syft_bg/test_cli.py Update CLI integration tests to patch SyftBgConfig.edit() and API list behavior.
packages/syft-bg/src/syft_bg/common/syft_bg_config.py Add SyftBgConfig.load()/edit() with file locking, preserve unknown YAML keys, and improve merge semantics.
packages/syft-bg/src/syft_bg/common/state.py Deduplicate file-locking by reusing shared file_lock().
packages/syft-bg/src/syft_bg/common/locking.py New shared file_lock() context manager for exclusive advisory locks.
packages/syft-bg/src/syft_bg/cli/commands.py Route CLI config mutations through SyftBgConfig.edit() and list via API.
packages/syft-bg/src/syft_bg/approve/orchestrator.py Switch orchestrator config load to SyftBgConfig.load().approve.
packages/syft-bg/src/syft_bg/approve/handlers/job.py Always read fresh auto-approvals from SyftBgConfig on each access.
packages/syft-bg/src/syft_bg/approve/config.py Remove AutoApproveConfig.load()/.save() YAML I/O methods.
packages/syft-bg/src/syft_bg/api/utils.py Adjust token move helper return semantics; route GCP project-id persistence via locked edit; widen resolve_content_files typing.
packages/syft-bg/src/syft_bg/api/api.py Refactor init token handling; ensure_running uses locked edit; harden auto_approve() with staging dir + atomic rename under lock.
notebooks/ai_audit/internal/test_email_e2e.ipynb Update notebook to load approve config via SyftBgConfig.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/syft-bg/src/syft_bg/api/api.py
Comment thread packages/syft-bg/src/syft_bg/api/api.py
@pjwerneck pjwerneck marked this pull request as ready for review July 8, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants