Skip to content

fix: validate persisted workflow modes#28

Open
KalleBylin wants to merge 2 commits intogastownhall:mainfrom
KalleBylin:fix/resolvemode-invalid-string
Open

fix: validate persisted workflow modes#28
KalleBylin wants to merge 2 commits intogastownhall:mainfrom
KalleBylin:fix/resolvemode-invalid-string

Conversation

@KalleBylin
Copy link
Copy Markdown
Contributor

Summary

This PR is a direct follow-up to #17, responding to the reviewer’s note about ResolveMode() allowing invalid persisted mode strings to pass through unchanged.

In #17, the default workflow mode behavior was clarified so unset mode resolves to pr, matching the intended behavior and README. During review, a related gap was called out: explicit invalid persisted values such as "typo" were still able to survive and reach downstream behavior. This PR addresses that issue directly.

The fix hardens workflow mode validation at the persistence / resolution boundary for both:

  • local file-backed wasteland config
  • hosted-mode metadata / resolver flow

What changed

  • Local file-backed config now validates mode on load and save.
  • Hosted auth connect / join now reject invalid explicit mode values before persisting metadata.
  • Hosted workspace resolution now rejects stale invalid hosted metadata before building SDK/backend clients.
  • Validation is shared through a common mode validator so local config, hosted metadata, CLI config setting, and API settings use the same rule.

Why this approach

The problem here is not just input validation at one setter. The real risk is invalid persisted state surviving long enough to produce inconsistent downstream behavior.

This change therefore hardens the config boundary itself:

  • unset mode still preserves the intended default behavior ("" => pr)
  • explicit invalid values now fail clearly instead of being silently accepted, normalized, or only breaking later in mode-dependent flows

That keeps valid pr and wild-west behavior unchanged while making invalid persisted state fail loudly and deterministically.

Hosted rationale

While implementing the local config fix, I found the same workflow-mode concept persisted separately in hosted metadata. The hosted connect / join path and hosted resolver had the same class of problem: invalid explicit values could still be stored and only fail later with mixed semantics.

Because this PR is specifically about hardening persisted workflow-mode handling, I included the hosted path in the same change rather than leaving local and hosted behavior inconsistent.

Tests

Added / updated coverage for:

  • invalid local config mode rejected on load/save
  • CLI surfacing invalid local config
  • join/create no longer swallowing config load failures
  • invalid hosted connect mode rejected
  • invalid hosted join mode rejected
  • stale invalid hosted metadata rejected during workspace resolution

Verification

Passed:

  • go test ./internal/hosted ./internal/api ./internal/federation ./cmd/wl
  • go vet ./...
  • go test ./...

Not verified via repo wrapper:

  • make check still fails in the existing golangci-lint step with:
    context loading failed: no go files to analyze

Follow-up

While working on this, I also noticed a separate local-config enumeration issue: ResolveConfig / wl list / wl doctor still reason from raw filenames returned by List() before each config is validated on load. That means invalid config files can still influence ambiguity / joined-count UX before the load error is surfaced.

I am intentionally leaving that as a follow-up so this PR stays focused on the reviewer-raised invalid-mode hardening issue and the matching hosted resolver boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant