Skip to content

Commit e1e438b

Browse files
SantiagoDePoloniaSantiagoDePolonia
committed
Merge remote-tracking branch 'origin/main' into refactor/internal-translated-executor
2 parents 17c8d6d + 3f81d47 commit e1e438b

11 files changed

Lines changed: 215 additions & 36 deletions

File tree

‎PossibleRefactoring.md‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,119 @@ Suggested action:
8686
- Introduce a small shared internal package for cache semantics.
8787
- Do it only if it can be done without creating import cycles.
8888

89+
## 6. Centralize fallback-mode semantics in `config`
90+
91+
Effort: low
92+
Risk: low
93+
94+
Why:
95+
- `config.ResolveFallbackDefaultMode()` now owns the blank-to-`auto` defaulting rule.
96+
- `internal/app/app.go` still re-implements fallback-mode parsing in:
97+
- `dashboardFallbackModeValue()`
98+
- `fallbackFeatureEnabledGlobally()`
99+
- `fallbackModeEnabled()`
100+
- Those helpers currently perform their own `TrimSpace` / case-folding instead of reusing config-owned semantics.
101+
102+
Suggested action:
103+
- Add small config-owned helpers for:
104+
- "is fallback enabled for this mode?"
105+
- "what dashboard mode should be exposed for this config?"
106+
- Remove the ad hoc mode parsing from `internal/app/app.go`.
107+
- This keeps blank, mixed-case, and future mode handling in one place.
108+
109+
## 7. Collapse the duplicated translated fallback attempt loops
110+
111+
Effort: medium
112+
Risk: medium
113+
114+
Why:
115+
- `internal/server/translated_inference_service.go` has two near-identical fallback loops:
116+
- `tryFallbackResponse()`
117+
- `tryFallbackStream()`
118+
- Both:
119+
- fetch selectors
120+
- gate on `shouldAttemptFallback()`
121+
- derive `providerType`
122+
- log attempt/success messages
123+
- walk candidates while preserving the last error
124+
125+
Suggested action:
126+
- Extract one shared iterator that owns:
127+
- selector traversal
128+
- provider-type lookup
129+
- attempt logging
130+
- last-error handling
131+
- Keep the typed wrappers only for the response/stream result shapes.
132+
133+
## 8. Precompute fallback source identity once per resolution
134+
135+
Effort: medium
136+
Risk: low to medium
137+
138+
Why:
139+
- `internal/fallback/resolver.go` recomputes trimmed selector identity several times per request:
140+
- `sourceModelInfo()`
141+
- `modeFor()`
142+
- `manualSelectorsFor()`
143+
- `matchKeys()`
144+
- `sourceKey()`
145+
- `modeFor()` and `manualSelectorsFor()` each rebuild the same ordered match-key list.
146+
147+
Suggested action:
148+
- Introduce a small internal struct for one fallback resolution pass, containing:
149+
- source model info
150+
- canonical source key
151+
- ordered match keys
152+
- Build it once in `ResolveFallbacks()` and pass it through helper calls.
153+
- This would trim repeated string cleanup and make precedence rules easier to inspect.
154+
155+
## 9. Extract manual fallback-rule file parsing from `loadFallbackConfig`
156+
157+
Effort: medium
158+
Risk: low to medium
159+
160+
Why:
161+
- `config.loadFallbackConfig()` currently owns both:
162+
- fallback-mode validation/defaulting
163+
- the custom JSON loader for `manual_rules_path`
164+
- The manual loader includes:
165+
- duplicate raw JSON key detection
166+
- `null` rejection
167+
- trailing-content validation
168+
- whitespace normalization
169+
- That makes the config loader harder to scan than the rest of the config pipeline.
170+
171+
Suggested action:
172+
- Move the manual-rule JSON parsing into a dedicated helper or file, for example `loadFallbackManualRules(path string)`.
173+
- Keep `loadFallbackConfig()` focused on policy validation and wiring.
174+
- Preserve the current strict error messages and test coverage while isolating the parser.
175+
176+
## 10. Pick one owner for execution-plan fallback defaults
177+
178+
Effort: medium
179+
Risk: medium
180+
181+
Why:
182+
- The managed backend default is set in `internal/app/app.go:defaultExecutionPlanInput()`.
183+
- The dashboard draft default is separately hardcoded in `internal/admin/dashboard/static/js/modules/execution-plans.js:defaultExecutionPlanForm()`.
184+
- We already changed both once to keep them aligned, which confirms the drift risk is real.
185+
186+
Suggested action:
187+
- Prefer a single server-owned default surface for execution-plan authoring defaults.
188+
- Options:
189+
- expose default feature flags from the admin config endpoint
190+
- derive the initial dashboard form from the active managed default workflow
191+
- This reduces UI/backend drift for fallback and other execution-plan features.
192+
89193
## Recommended order
90194

91195
1. Remove `CacheTypeBoth`.
92196
2. Deduplicate the dashboard empty-state object.
93197
3. Keep cached-only policy in one layer.
94198
4. Remove the legacy middleware path.
95199
5. Centralize cache semantics in a shared package.
200+
6. Centralize fallback-mode semantics in `config`.
201+
7. Collapse the duplicated translated fallback attempt loops.
202+
8. Precompute fallback source identity once per resolution.
203+
9. Extract manual fallback-rule file parsing from `loadFallbackConfig`.
204+
10. Pick one owner for execution-plan fallback defaults.

‎config/config.example.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ guardrails:
153153
# # prompt: "Custom rewrite instructions here."
154154

155155
fallback:
156-
default_mode: "off" # "off", "manual", or "auto"
156+
default_mode: "auto" # "off", "manual", or "auto"
157157
manual_rules_path: "config/fallback.example.json" # optional JSON map: {"model": ["fallback-1", "provider/model"]}; required when fallback.default_mode or any fallback.overrides.*.mode is "manual"
158158
overrides:
159159
"gpt-4o":

‎config/config.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,16 @@ func normalizeFallbackMode(mode FallbackMode) FallbackMode {
114114
return FallbackMode(strings.ToLower(strings.TrimSpace(string(mode))))
115115
}
116116

117+
// ResolveFallbackDefaultMode canonicalizes the global fallback default mode and
118+
// applies the process default when unset.
119+
func ResolveFallbackDefaultMode(mode FallbackMode) FallbackMode {
120+
mode = normalizeFallbackMode(mode)
121+
if mode == "" {
122+
return FallbackModeAuto
123+
}
124+
return mode
125+
}
126+
117127
// FallbackModelOverride holds per-model mode overrides.
118128
type FallbackModelOverride struct {
119129
Mode FallbackMode `yaml:"mode" json:"mode"`
@@ -122,7 +132,7 @@ type FallbackModelOverride struct {
122132
// FallbackConfig holds translated-route model fallback policy.
123133
type FallbackConfig struct {
124134
// DefaultMode controls the fallback behavior when no per-model override exists.
125-
// Supported values: "auto", "manual", "off". Default: "off".
135+
// Supported values: "auto", "manual", "off". Default: "auto".
126136
DefaultMode FallbackMode `yaml:"default_mode" env:"FEATURE_FALLBACK_MODE"`
127137

128138
// ManualRulesPath points to a JSON file that maps source model selectors to
@@ -900,7 +910,7 @@ func buildDefaultConfig() *Config {
900910
ResponseHeaderTimeout: 600,
901911
},
902912
Fallback: FallbackConfig{
903-
DefaultMode: FallbackModeOff,
913+
DefaultMode: FallbackModeAuto,
904914
},
905915
ExecutionPlans: ExecutionPlansConfig{
906916
RefreshInterval: time.Minute,
@@ -1018,10 +1028,7 @@ func loadFallbackConfig(cfg *FallbackConfig) error {
10181028
return nil
10191029
}
10201030

1021-
cfg.DefaultMode = normalizeFallbackMode(cfg.DefaultMode)
1022-
if cfg.DefaultMode == "" {
1023-
cfg.DefaultMode = FallbackModeOff
1024-
}
1031+
cfg.DefaultMode = ResolveFallbackDefaultMode(cfg.DefaultMode)
10251032
if !cfg.DefaultMode.Valid() {
10261033
return fmt.Errorf("fallback.default_mode must be one of: auto, manual, off")
10271034
}

‎config/config_test.go‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,8 @@ func TestBuildDefaultConfig(t *testing.T) {
168168
if cfg.Guardrails.EnableForBatchProcessing {
169169
t.Error("expected Guardrails.EnableForBatchProcessing=false")
170170
}
171-
if cfg.Fallback.DefaultMode != FallbackModeOff {
172-
t.Errorf("expected Fallback.DefaultMode=off, got %q", cfg.Fallback.DefaultMode)
171+
if cfg.Fallback.DefaultMode != FallbackModeAuto {
172+
t.Errorf("expected Fallback.DefaultMode=auto, got %q", cfg.Fallback.DefaultMode)
173173
}
174174
if cfg.Cache.Response.Simple != nil {
175175
t.Errorf("expected Cache.Response.Simple=nil in defaults, got %+v", cfg.Cache.Response.Simple)
@@ -570,6 +570,28 @@ func TestLoad_FeatureFallbackModeEnvOverridesFallbackDefaultMode(t *testing.T) {
570570
})
571571
}
572572

573+
func TestLoad_BlankFallbackDefaultModeResolvesToAuto(t *testing.T) {
574+
clearAllConfigEnvVars(t)
575+
576+
withTempDir(t, func(dir string) {
577+
yaml := `
578+
fallback:
579+
default_mode: ""
580+
`
581+
if err := os.WriteFile(filepath.Join(dir, "config.yaml"), []byte(yaml), 0644); err != nil {
582+
t.Fatalf("Failed to write config.yaml: %v", err)
583+
}
584+
585+
result, err := Load()
586+
if err != nil {
587+
t.Fatalf("Load() failed: %v", err)
588+
}
589+
if result.Config.Fallback.DefaultMode != FallbackModeAuto {
590+
t.Fatalf("Fallback.DefaultMode = %q, want %q", result.Config.Fallback.DefaultMode, FallbackModeAuto)
591+
}
592+
})
593+
}
594+
573595
func TestLoad_PassthroughFlags_EnvOverridesYAML(t *testing.T) {
574596
tests := []struct {
575597
name string

‎internal/admin/dashboard/static/css/dashboard.css‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3154,14 +3154,6 @@ body.conversation-drawer-open {
31543154
font-weight: 700;
31553155
}
31563156

3157-
.ep-node-async-usage {
3158-
--accent: var(--success);
3159-
}
3160-
3161-
.ep-node-async-audit {
3162-
--accent: var(--warning);
3163-
}
3164-
31653157
/* "ASYNC" label inline on the right of the branch */
31663158
.ep-async-label {
31673159
display: inline-flex;

‎internal/admin/dashboard/static/js/modules/execution-plans-layout.test.js‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,17 @@ test('async label stays inline on the right side of the branch', () => {
6262
);
6363
assert.match(
6464
template,
65-
/<div class="ep-conn ep-conn-async" x-show="{{\.}}\.showAudit"><\/div>\s*<div class="ep-node ep-node-feature ep-node-async ep-node-async-audit" x-show="{{\.}}\.showAudit">/
65+
/<div class="ep-conn ep-conn-async" x-show="{{\.}}\.showAudit"><\/div>\s*<div class="ep-node ep-node-feature ep-node-async ep-node-async-audit" x-show="{{\.}}\.showAudit" :class="{{\.}}\.auditNodeClass">/
66+
);
67+
assert.match(
68+
template,
69+
/<div class="ep-node ep-node-feature ep-node-async ep-node-async-usage" x-show="{{\.}}\.showUsage" :class="{{\.}}\.usageNodeClass">/
6670
);
6771

6872
const asyncLabelRule = readCSSRule(css, '.ep-async-label');
6973
assert.doesNotMatch(asyncLabelRule, /position:\s*absolute/);
70-
71-
const asyncUsageRule = readCSSRule(css, '.ep-node-async-usage');
72-
assert.match(asyncUsageRule, /--accent:\s*var\(--success\)/);
73-
74-
const asyncAuditRule = readCSSRule(css, '.ep-node-async-audit');
75-
assert.match(asyncAuditRule, /--accent:\s*var\(--warning\)/);
74+
assert.doesNotMatch(css, /\.ep-node-async-usage\s*\{/);
75+
assert.doesNotMatch(css, /\.ep-node-async-audit\s*\{/);
7676
});
7777

7878
test('workflow nodes use endpoint and feature color groups consistently', () => {

‎internal/admin/dashboard/static/js/modules/execution-plans.js‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
audit: true,
3535
usage: true,
3636
guardrails: false,
37-
fallback: false
37+
fallback: true
3838
},
3939
guardrails: []
4040
},
@@ -51,7 +51,7 @@
5151
audit: true,
5252
usage: true,
5353
guardrails: false,
54-
fallback: false
54+
fallback: true
5555
},
5656
guardrails: []
5757
};
@@ -1091,6 +1091,7 @@
10911091
? this.executionPlanNormalizedFeatures(config.features)
10921092
: this.executionPlanSourceFeatures(source);
10931093
const forceAudit = !!config.forceAudit;
1094+
const highlightAsyncPresent = !!config.highlightAsyncPresent;
10941095
const showGuardrails = !!features.guardrails;
10951096
const showUsage = !!features.usage;
10961097
const showAudit = forceAudit || !!features.audit;
@@ -1111,6 +1112,8 @@
11111112
responseNodeClass: this.epResponseNodeClass(runtime),
11121113
authNodeClass: this.epAuthNodeClass(runtime),
11131114
authNodeSublabel: this.epAuthNodeSublabel(runtime),
1115+
usageNodeClass: this.epAsyncNodeClass(showUsage, highlightAsyncPresent),
1116+
auditNodeClass: this.epAsyncNodeClass(showAudit, highlightAsyncPresent),
11141117
showAsync,
11151118
showUsage,
11161119
showAudit,
@@ -1130,7 +1133,8 @@
11301133
entry,
11311134
features,
11321135
forceAudit: true,
1133-
forceAsync: true
1136+
forceAsync: true,
1137+
highlightAsyncPresent: true
11341138
});
11351139
},
11361140

@@ -1200,6 +1204,10 @@
12001204
return runtime.authMethod;
12011205
},
12021206

1207+
epAsyncNodeClass(visible, highlightPresent) {
1208+
return visible && highlightPresent ? 'ep-node-success' : '';
1209+
},
1210+
12031211
epRuntimeFromEntry(entry) {
12041212
if (!entry) return null;
12051213
const normalizedCacheType = (() => {

‎internal/admin/dashboard/static/js/modules/execution-plans.test.js‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,11 @@ test('executionPlanProviderOptions returns unique sorted provider types', () =>
3838
);
3939
});
4040

41-
test('defaultExecutionPlanForm starts fallback disabled for new workflows', () => {
41+
test('defaultExecutionPlanForm starts fallback enabled for new workflows', () => {
4242
const module = createExecutionPlansModule();
4343

44-
assert.equal(module.executionPlanForm.features.fallback, false);
45-
assert.equal(module.defaultExecutionPlanForm().features.fallback, false);
44+
assert.equal(module.executionPlanForm.features.fallback, true);
45+
assert.equal(module.defaultExecutionPlanForm().features.fallback, true);
4646
});
4747

4848
test('executionPlanPreview mirrors the draft workflow card state from the editor form', () => {
@@ -203,6 +203,8 @@ test('executionPlanWorkflowChart returns the shared chart contract for workflow
203203
responseNodeClass: '',
204204
authNodeClass: '',
205205
authNodeSublabel: null,
206+
usageNodeClass: '',
207+
auditNodeClass: '',
206208
showAsync: true,
207209
showUsage: false,
208210
showAudit: true,
@@ -256,6 +258,8 @@ test('executionPlanWorkflowChart masks globally disabled workflow features from
256258
responseNodeClass: '',
257259
authNodeClass: '',
258260
authNodeSublabel: null,
261+
usageNodeClass: '',
262+
auditNodeClass: '',
259263
showAsync: false,
260264
showUsage: false,
261265
showAudit: false,
@@ -330,6 +334,8 @@ test('executionPlanAuditChart returns the shared chart contract for audit runtim
330334
responseNodeClass: 'ep-node-success',
331335
authNodeClass: '',
332336
authNodeSublabel: null,
337+
usageNodeClass: 'ep-node-success',
338+
auditNodeClass: 'ep-node-success',
333339
showAsync: true,
334340
showUsage: true,
335341
showAudit: true,
@@ -364,6 +370,8 @@ test('executionPlanAuditChart forces audit nodes even when the workflow version
364370
responseNodeClass: 'ep-node-success',
365371
authNodeClass: '',
366372
authNodeSublabel: null,
373+
usageNodeClass: '',
374+
auditNodeClass: 'ep-node-success',
367375
showAsync: true,
368376
showUsage: false,
369377
showAudit: true,
@@ -427,6 +435,8 @@ test('executionPlanAuditChart prefers request-time execution features over curre
427435
responseNodeClass: 'ep-node-success',
428436
authNodeClass: '',
429437
authNodeSublabel: null,
438+
usageNodeClass: '',
439+
auditNodeClass: 'ep-node-success',
430440
showAsync: true,
431441
showUsage: false,
432442
showAudit: true,
@@ -435,6 +445,14 @@ test('executionPlanAuditChart prefers request-time execution features over curre
435445
);
436446
});
437447

448+
test('epAsyncNodeClass only marks async nodes green when the audit-log override is enabled', () => {
449+
const module = createExecutionPlansModule();
450+
451+
assert.equal(module.epAsyncNodeClass(true, false), '');
452+
assert.equal(module.epAsyncNodeClass(false, true), '');
453+
assert.equal(module.epAsyncNodeClass(true, true), 'ep-node-success');
454+
});
455+
438456
test('executionPlanSubmitMode switches to save when an active workflow already matches the selected scope', () => {
439457
const module = createExecutionPlansModule();
440458
module.executionPlans = [

0 commit comments

Comments
 (0)