Repository navigation
feat(workspace): loud-warn when fallback masks a .env override - #1369
Merged
Merged
Conversation
When a process resolves the workspace without `SUTANDO_WORKSPACE` in its env but `.env` defines the var, the fallback silently lands in `~/.sutando/workspace/` while the rest of the fleet uses the override. Two services with the same `.env` then split-brain — one writes to `~/.sutando/workspace/state/agent-registry.json`, another reads from `.env`-specified `state/agent-registry.json`. The bug is invisible: no error, just empty/stale state. Adds a one-line stderr warning that fires once per process when the fallback masks a .env override. The actual resolution behavior does NOT change — callers who genuinely want the default still get it (no .env override → no warn → fallback as before). Applied to all three workspace resolvers: - `src/workspace_default.py` (`resolve_workspace`) - `src/workspace_default.ts` (`resolveWorkspace`) - `src/init.sh` (inline `WORKSPACE=...` block) The Python and TS resolvers add a small `_grep_env_for_workspace()` helper that walks up from the module file to find `.env` and reads the `SUTANDO_WORKSPACE=` line (best-effort, never raises). init.sh uses inline `grep | cut | sed` against `$REPO/.env`. Behavior is parity across all three. Companion to: - #1367 (startup.sh ordering — moved .env source before init.sh) - #1368 (agent-registry — auto-honors .env when env var missing, Option B; this PR is Option C "loud warn" from the discussion) Tier 1A of the prevention plan owner greenlit: Tier 1A: loud-warn (this PR) Tier 1B: clean-shell CI smoke test (separate PR, follow-up) Tier 2: unified `scripts/source-env.sh` helper + manifest (later) Tier 3: drop default fallback entirely (long-term) Verified locally across 6 scenarios: - env var set → no warn (PY/TS/SH) - env unset + .env override → WARN fires (PY/TS/SH) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
left a comment
Collaborator
There was a problem hiding this comment.
Review — feat(workspace) PR #1369
BLOCKING
None.
NON-BLOCKING
- 3-file parity (py + ts + sh) — same helper now lives in
workspace_default.py,workspace_default.ts, and inline ininit.sh. Future edits must update all three or the warning conditions drift. A shared spec indocs/workspace-contract.md(or a comment header in one of the three pointing at the others) would lower the drift risk. Could also be captured by Tier 1B (clean-shell CI smoke). init.shuseshead -1so multipleSUTANDO_WORKSPACE=lines silently take the first match. Matches the python+ts behavior (also stop at first match within the first.envfound). Document this as intentional, or make it loud if mismatch detected — but as written it's consistent and that consistency is what matters.- TS
replace(/^~/, homedir())only handles leading bare~, not~userform. Matches whatinit.shdoes (${val/#\~/$HOME}) and the pythonPath(val).expanduser()(also bare~only). Consistent across the trio. Fine. - Warn-only vs auto-honor decision is well-justified in the PR body —
src/runs inside processes that could source.envthemselves, so warn forces the upstream fix;_workspace_resolve.pyin agent-registry is the auto-honor path for the SessionStart hook where sourcing isn't realistic. Clean split.
The _FALLBACK_WARN_PRINTED module-level guard (py) / _fallbackWarnPrinted (ts) ensures one warning per process. Looks right.
— Maddy
2 tasks
Collaborator
Author
|
Thanks Maddy — all four points heard. On the 3-file parity (py + ts + sh): agree the drift risk is real and worth tracking. I can either:
Lean toward (b) if owner doesn't object — cheapest mitigation, lives at the site that needs to be touched together. Standing by for the call. The other three (head -1, ~ form, warn-only split) are all consistency observations matching the trio's existing behavior — no change planned. |
Contributor
|
@cla-assistant check |
john-the-dev
added a commit
to john-the-dev/sutando
that referenced
this pull request
Jun 11, 2026
…kspace_default Part of issue sonichi#1378 sub-item 1 (workspace-resolution dedup). Replaces the local 5-line copy of resolve_workspace() with the canonical import from src/workspace_default.py — the same helper health-check.py, dashboard.py, dm-result.py, and others already use. Benefits: a rule change (e.g. the loud-warn-on-fallback added in sonichi#1369) now applies to obsidian-mirror automatically; only one place to maintain. Co-Authored-By: Claude Sonnet 4.6 <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
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
When a process resolves the workspace without
SUTANDO_WORKSPACEin its env but.envdefines the var, the fallback silently lands in~/.sutando/workspace/while the rest of the fleet uses the override. Two services with the same.envthen split-brain — one writes to~/.sutando/workspace/state/..., another reads from.env-specifiedstate/.... Invisible: no error, just empty/stale state. (This is exactly the bug class that produced #1367 instartup.shand #1368 inskills/agent-registry/.)This PR adds a one-line stderr warning that fires once per process when the fallback masks a
.envoverride. Resolution behavior does NOT change — callers who genuinely want the default still get it (no.envoverride → no warn → fallback as before).What changes
Applied to all three workspace resolvers — kept in parity:
src/workspace_default.py— adds_grep_env_for_workspace()helper + warn block inresolve_workspace().src/workspace_default.ts— addsgrepEnvForWorkspace()helper + warn block inresolveWorkspace().src/init.sh— inline grep/cut/sed of$REPO/.env+ warn block in the workspace-resolution branch.The warn message looks like:
Behavior matrix:
.envfoundWhy "warn-only" (not auto-honor)
skills/agent-registry/scripts/_workspace_resolve.py(#1368) is Option B — it auto-honors the.envvalue when the env var is missing. That's the right behavior for an out-of-band hook (SessionStart) where there's no realistic way to source.envfirst.The src/ resolvers are different: they run inside processes that could have sourced
.envthemselves. Auto-honoring would mask the upstream bug ("why didn't the bootstrap source.env?"). Warn-only forces the upstream fix while keeping the existing fallback semantics.Tier 1A of the prevention plan
Owner greenlit four tiers earlier today:
scripts/source-env.shhelper + per-script env manifestTest plan
Verified locally across 6 scenarios — all pass:
.envoverride / PY → WARN ✅.envoverride / TS → WARN ✅.envoverride / SH → WARN ✅Reviewers can repro by
env -i HOME=/tmp/x PATH=$PATH bash -c "python3 -c 'from src.workspace_default import resolve_workspace; resolve_workspace(migrate=False)'"from a checkout with.envcontainingSUTANDO_WORKSPACE=....Related
🤖 Generated with Claude Code