refactor: decouple UpstashDriver from env loading (Item 15)

[koladefaj]

This PR resolves Item 15 of the Technical Debt Roadmap. It decouples the UpstashDriver from global environment loading, moving configuration management to the application entry points.

Changes

• Decoupled Service Layer: Removed module-level load_dotenv() from autograder/services/upstash_driver.py to eliminate side effects at import time.

• Constructor Injection: Refactored UpstashDriver to accept redis_url and redis_token as explicit parameters in init.

Startup Configuration:

• Integrated load_dotenv() into the FastAPI lifespan in web/core/lifespan.py.

• Verified existing load_dotenv() usage in the GitHub Action entry point (github_action/main.py).

• Robustness: Updated StepRegistry and UpstashDriver to handle missing environment variables gracefully by providing fallbacks and raising descriptive ValueError exceptions.

• Test Suite Maintenance: Updated unit tests for UpstashDriver and StepRegistry to use mocks and ANY matchers, ensuring tests pass without requiring a local .env file.

Impact
This change makes the codebase more predictable, significantly easier to test, and aligns with the project's move toward a more decoupled, pipeline-based architecture.

Testing Performed
Ran pytest on updated driver and registry tests.

Verified that importing the driver module no longer triggers environment loading side effects.

[ArthurCRodrigues]

Hi @koladefaj sorry for the delay on your answers. I'm really busy so i'll have @matheusmra assist you now on.

@matheusmra could you review, please?

[matheusmra]

Well done, @koladefaj.
Once you’ve completed the requested changes, this PR will be ready to merge.

[matheusmra]

This file doesn’t need to be committed.
Could you please remove it?

[matheusmra]

Suggestion: The fallback in os.getenv is unnecessary here.

UpstashDriver.__init__ already validates the credentials and raises a descriptive ValueError if redis_url or redis_token are missing or empty.

[matheusmra]

Can you update this comment to match the changes that I asked on autograder/steps/step_registry.py?

[matheusmra]

One thing worth noting: UpstashDriver currently has two responsibilities it implements the Exporter interface for the pipeline (export, set_score) and also manages user accounts/quotas (get_user_quota, decrement_user_quota, create_user, user_exists). These are different concerns serving different consumers (pipeline vs. web layer).

Maybe we could move the user/quota methods into a separate UpstashUserService in web/services/, leaving UpstashDriver as a focused, minimal Exporter. Since those methods currently have no callers outside the file itself, the split would be zero-risk.
Let me know what you think @koladefaj

[koladefaj]

One thing worth noting: UpstashDriver currently has two responsibilities it implements the Exporter interface for the pipeline (export, set_score) and also manages user accounts/quotas (get_user_quota, decrement_user_quota, create_user, user_exists). These are different concerns serving different consumers (pipeline vs. web layer).

Maybe we could move the user/quota methods into a separate UpstashUserService in web/services/, leaving UpstashDriver as a focused, minimal Exporter. Since those methods currently have no callers outside the file itself, the split would be zero-risk. Let me know what you think @koladefaj

That’s a great catch, @matheusmra. It definitely feels cleaner to separate the pipeline's export logic from the user management stuff. Keeping the driver focused on being an Exporter while moving the quota methods to a dedicated UpstashUserService makes the code much more intuitive. I'll handle that split in my next push, along with the other minor fixes you pointed out. Thanks for the guidance

[matheusmra]

One thing worth noting: UpstashDriver currently has two responsibilities it implements the Exporter interface for the pipeline (export, set_score) and also manages user accounts/quotas (get_user_quota, decrement_user_quota, create_user, user_exists). These are different concerns serving different consumers (pipeline vs. web layer).
Maybe we could move the user/quota methods into a separate UpstashUserService in web/services/, leaving UpstashDriver as a focused, minimal Exporter. Since those methods currently have no callers outside the file itself, the split would be zero-risk. Let me know what you think @koladefaj

That’s a great catch, @matheusmra. It definitely feels cleaner to separate the pipeline's export logic from the user management stuff. Keeping the driver focused on being an Exporter while moving the quota methods to a dedicated UpstashUserService makes the code much more intuitive. I'll handle that split in my next push, along with the other minor fixes you pointed out. Thanks for the guidance

Actually @koladefaj, we can’t split this logic right now. Even though they’re not referenced anywhere in the code, we’ll be using them in the future in our github action.

[koladefaj]

One thing worth noting: UpstashDriver currently has two responsibilities it implements the Exporter interface for the pipeline (export, set_score) and also manages user accounts/quotas (get_user_quota, decrement_user_quota, create_user, user_exists). These are different concerns serving different consumers (pipeline vs. web layer).
Maybe we could move the user/quota methods into a separate UpstashUserService in web/services/, leaving UpstashDriver as a focused, minimal Exporter. Since those methods currently have no callers outside the file itself, the split would be zero-risk. Let me know what you think @koladefaj

That’s a great catch, @matheusmra. It definitely feels cleaner to separate the pipeline's export logic from the user management stuff. Keeping the driver focused on being an Exporter while moving the quota methods to a dedicated UpstashUserService makes the code much more intuitive. I'll handle that split in my next push, along with the other minor fixes you pointed out. Thanks for the guidance

Actually @koladefaj, we can’t split this logic right now. Even though they’re not referenced anywhere in the code, we’ll be using them in the future in our github action.

No problem, @matheusmra I'll keep the logic in UpstashDriver then to ensure it's ready for the GitHub Action integration.
I'll go ahead and just push the fixes for the other points, removing the os.getenv fallbacks, updating the test comments, and removing that accidental JSON file. Thanks for the heads-up

[koladefaj]

I've pushed the requested changes, @matheusmra
Env Logic: Simplified the os.getenv calls to remove the fallbacks.
Testing: Updated the unit test comments to reflect the simplified logic and driver-level validation.
Architecture: Kept the user/quota logic in the driver as requested for the future GitHub Action.

[matheusmra]

I've pushed the requested changes, @matheusmra Env Logic: Simplified the os.getenv calls to remove the fallbacks. Testing: Updated the unit test comments to reflect the simplified logic and driver-level validation. Architecture: Kept the user/quota logic in the driver as requested for the future GitHub Action.

Thanks!