otto: stop setting AF_CONFIG=/dev/null in the no-Airflow fallback#2121
Open
jlaneve wants to merge 3 commits into
Open
otto: stop setting AF_CONFIG=/dev/null in the no-Airflow fallback#2121jlaneve wants to merge 3 commits into
jlaneve wants to merge 3 commits into
Conversation
…ected
The previous behavior blanked af's config view in the fallback case to
keep an unrelated global current-instance from leaking in. af 0.8.0's
layered config (project-local > project-shared > global) makes that
guard counterproductive: it hides the team's committed deployment
inventory in `<project>/.astro/config.yaml` from the model, so questions
like "what DAGs are failing on staging?" hit a dead-end "no instances
configured" wall even though the user has staging defined.
Drop the sentinel. af's project-aware resolution already does the right
thing inside a project, and outside one the resolved global instance is
generally what the user wants ("the deployment I last worked on" is a
reasonable default for an open-ended question).
The remaining risk — silently picking up a stale global current-instance
when running `astro otto` from a non-project dir — is better mitigated
by surfacing the resolved instance in Otto's UI (separate change in the
otto repo) than by blanking the config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 0Coverage decreased (-0.005%) to 39.701%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Drop the multi-line "previously we did X..." narrative; keep a tight WHY explaining the current contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both checks pinned the absence of an env var that the implementation no longer touches anywhere, which is something you could write for any of the env vars BuildEnv doesn't set. Not a useful regression guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
elsebranch inpkg/otto/config.go:BuildEnvsetAF_CONFIG=/dev/nullwhen otto couldn't match the cwd to a running astro-dev project. That blanked af's config view to prevent silent fallthrough to a globally-scopedcurrent-instancethat often pointed at whatever project the user last ranastro dev startin.af 0.8.0 (shipped in
astro-airflow-mcp0.8.0, picked up by Otto 0.1.3) introduced a layered config: project-local > project-shared > global, with project root discovered by walking up from cwd looking for.astro/. That makes the sentinel counterproductive in two cases:<project>/.astro/config.yamlmay have the team's committed deployment inventory (the headline use case for the layered config). Today the model sees "no instances configured" and dead-ends; without the sentinel it sees the team's staging/prod/etc. and can answer questions like "what DAGs are failing on staging?".current-instanceis typically a sensible default ("the Airflow I was last working with"), and surfacing the resolved instance to the user is a better safety net than blanking the config.Changes
pkg/otto/config.go: drop theelse { set("AF_CONFIG", os.DevNull) }block. Comment block explains the new contract: af owns instance resolution; otto's env-var injection still wins for the "active" Airflow when one was detected.pkg/otto/config_test.go: flipTestBuildEnv_SkipsEmptyValuesto assertAF_CONFIGis NOT set in the no-Airflow case.Why this is safe
AIRFLOW_API_URLinjection is unchanged. When otto detects a project Airflow, env vars beat per-instance config in af 0.8.0, so otto's deployment selection still wins for actual API calls.astro ottofrom a non-project dir with a stale global pointer — a case better solved by the otto-side change to surface the resolved instance in the UI.Test plan
go test ./pkg/otto/...passes (9 tests inConfigSuite)TestBuildEnv_SkipsEmptyValuesto assert the new behaviorastro ottofrom a non-project dir, ask "list my Airflow instances" → returns the layered-config view instead of "no instances"Follow-ups (separate PRs)
🤖 Generated with Claude Code