Skip to content

fix(flow): fail over on missing model errors#35

Merged
PhucHai123 merged 9 commits into
mainfrom
fix/flow-404-failover-usage-guards
Jul 2, 2026
Merged

fix(flow): fail over on missing model errors#35
PhucHai123 merged 9 commits into
mainfrom
fix/flow-404-failover-usage-guards

Conversation

@PhucHai123

@PhucHai123 PhucHai123 commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

PR #35 review follow-up

Closes every open review item from the post-merge PR #35 review in a single coordinated commit (4ae1397):

  • cycle-guard — replaced the regressed 2-shape trigger with a 4-shape trigger (JSON envelope, type marker, English phrase + 404 token, 404 token + /v\d+/... API-path context). Closes the asymmetry with NON_RETRYABLE_CONNECTION_PATTERNS and preserves the negative space (benign stderr text never triggers failover).
  • cache invalidation — moved invalidateModelsJsonCache() to writeFlowModelConfig (the only writer that actually touches the registry); removed wasted invalidations from writeFlowSetting / writeGlobalFlowMode. Lazy-loads the registry inside resolveFlowModelCandidates so empty strategies skip the disk read entirely and emit no spurious "Failed to read settings JSON" warn.
  • session_start wiringsrc/index.ts now calls invalidateModelsJsonCache() next to invalidateSettingsCache() so long-running daemons honor external models.json edits without restart.
  • fail-fast telemetry — leaves model: undefined in the synthesized SingleResult and in the onFlowMetrics call so per-model rollups are not poisoned. Missing-model list is preserved in stderr / errorMessage for diagnostics.
  • audit drift warnresolveAuditModel now emits a one-time logWarn when the requested primary differs from the effective failover candidate, so silent audit-model swaps (stale registry, provider rename) are operator-visible.
  • CI hardening — renamed concurrency group to release-bump-version with a 30-minute job timeout, shared the same group + timeout with publish.yml, and dropped publish.yml's redundant push: tags: 'v*' trigger that produced deterministic npm 409 against bump-version.yml's own publish step.
  • docsCONFIGURATION.md documents both the coalesced missing-model warning and the model: undefined contract on the fail-fast path.

Closes the PR #35 review.


Summary

Closes two long-standing failure modes in the flow runner:

  1. Provider-side misses (404 / resource_not_found_error / tool_call_id 400) used to surface as silent flows that exit 0 with no usable output. They now trigger failover to the next configured model, and snapshots are sized against the first valid candidate instead of the (possibly invalid) primary.
  2. All-invalid model configs used to crash deep inside runFlow with a confusing "model not found" wall. They now fail fast with a clear Bad settings: all configured ... are missing from models.json message, both in the flow runner and in the trace tool, with model left undefined so per-model telemetry stays clean.

Bundled hardening (CI safety, test isolation, telemetry) and every prior review follow-up gap is addressed across the nine commits.

Bundled hardening

Three small, low-risk hardening changes that sit directly adjacent to the touched files. Each is fully tested.

  • CI safety (6299ea5 + 4ae1397) — serializes manual Release runs via a release-bump-version concurrency group, caps each job at 30 minutes, pins actions/checkout to ref: main, and runs git fetch origin main && git checkout -B main FETCH_HEAD instead of reset --hard. publish.yml shares the same group + timeout and dropped its redundant push: tags: 'v*' trigger that produced a deterministic npm 409 against bump-version.yml's own publish step.
  • Test isolation (b9815db) — isolates the tilde expansion batch tests under a fake HOME, and the tilde read-back uses process.env.HOME ?? os.homedir() so Windows / CI containers without POSIX os.homedir() reading HOME no longer risk touching the real user home. Pure test hygiene; no production behavior change.
  • Flow telemetry (7e41f70) — aggregateFlowUsage now guards against undefined/null r.usage and missing fields so cost/turns no longer become NaN when upstream omits them.

What changed — by concern

Flow execution (src/flow/)

  • cycle-guard.tsshouldFailover now retries on a 4-shape trigger: the canonical JSON envelope ("type":"resource_not_found_error"), the bare machine type marker (resource_not_found_error), the English phrase "requested resource was not found" paired with a literal 404 token, and a bare 404 token when stderr also contains a /v\d+/... API-path. Bare English phrasing without co-occurring tokens is not treated as a 404 — benign stderr (tool output, docs) cannot trigger failover. Closes the asymmetry with NON_RETRYABLE_CONNECTION_PATTERNS where 404 already denies connection retries.
  • execute-single.ts — fails fast with a clear Bad settings error before invoking runFlow when every configured model is missing from models.json. Emits onFlowMetrics for downstream accounting, but leaves result.model and metrics.model undefined so per-model rollups are not poisoned. Missing-model list is preserved in stderr / errorMessage for diagnostics.
  • audit-formatters.ts — selects the audit model from effectivePrimary (first valid candidate), and emits a one-time logWarn if the resolved model drifts from the originally requested one (stale registry, provider rename).
  • executor.ts — drops dead imports of resolveFlowModelCandidates and resolveModelContextWindow.

Configuration & model registry (src/config/, src/index.ts)

  • config.ts
    • resolveFlowModelCandidates returns invalidCandidates and effectivePrimary (first known-good model), and lazy-loads the registry via a getRegistry() closure. Empty strategies (no flowModel / cliTierOverride / strategy[tier] / fallbackModel) skip the disk read entirely and emit no spurious "Failed to read settings JSON" warn in fresh-tmpDir environments.
    • writeFlowModelConfig invalidates _modelsJsonCache (the only writer that actually mutates the configured-model set). writeFlowSetting / writeGlobalFlowMode no longer trigger wasted invalidations.
    • getModelsJsonPath import dropped (no longer used directly from this module).
    • Missing-model warning is consolidated into a single summary per resolution instead of one logWarn per invalid candidate.
  • models.ts — exports getModelsJsonPath, exports the cached readModelsJson() reader, exports hasConfiguredModel, and exposes invalidateModelsJsonCache() for tests and hot-reload boundaries.
  • index.tssession_start now calls invalidateModelsJsonCache() next to invalidateSettingsCache() so long-running daemons honor external models.json edits without restart.

Trace runtime (src/tools/trace.ts)

  • Same fail-fast Bad settings behavior when all configured trace models are missing.
  • Selects effectivePrimary for the runtime model and reuses the cached resolveModelContextWindow lookup.

Types & entry (src/types/flow.ts, src/index.ts)

  • flow.tsaggregateFlowUsage guards against undefined/null r.usage and missing fields (input, output, cost, turns, toolCalls, cacheRead, cacheWrite, smoothedTps). Cost / turns no longer become NaN when upstream omits them.
  • index.ts — sizes the context-window budget against effectivePrimary so that an invalid primary no longer poisons the snapshot window.

CI (.github/workflows/)

  • bump-version.ymlconcurrency: release-bump-version (renamed from release-main), cancel-in-progress: false, timeout-minutes: 30 on the release job. actions/checkout pinned to ref: main. Sync latest main runs git fetch origin main && git checkout -B main FETCH_HEAD.
  • publish.yml — joins the same release-bump-version group with a matching timeout-minutes: 30. Drops the redundant push: tags: 'v*' trigger that produced a deterministic npm 409 "You cannot publish over the previously published version" against bump-version.yml's own publish step. Head-of-file comment documents the rationale.

Docs (docs/CONFIGURATION.md)

  • Documents the coalesced missing-model warning (N configured flow model(s) are not present in models.json; trying them anyway: a, b, c).
  • Documents the model: undefined contract on the fail-fast path so per-model rollups are not poisoned.

Tests added / updated

  • tests/cycle-guard.test.ts — six new test cases for the 4-shape trigger (JSON envelope with / without 404 digit, bare machine type marker, English phrase + 404 token, bare 404 + API path; negative cases for bare 404 in tool output and complete-flow 404). Plus the existing positive case for provider 404.
  • tests/execute-single-invalid-models.test.ts — fail-fast path leaves model undefined and forwards undefined model to onFlowMetrics; runFlow must not be called. Updated existing assertions to match the new contract.
  • tests/trace-runtime.test.ts — partial-valid trace runtime picks the first valid candidate; all-missing throws the same Bad settings error.
  • tests/audit-formatters.test.tseffectivePrimary selects failover when primary is invalid; returns undefined only when all candidates are invalid; emits the new drift warn on positive case; suppresses it on negative case.
  • tests/config.test.ts — extensive new cache invalidation boundaries suite. Asserts that writeFlowSetting does NOT clear _modelsJsonCache, that writeFlowModelConfig DOES, and that resolveFlowModelCandidates with an empty strategy emits no "Failed to read settings JSON" warn. Updated for the new return shape (invalidCandidates, effectivePrimary).
  • tests/types.test.tsaggregateFlowUsage does not produce NaN when cost is omitted.
  • tests/batch.test.ts — isolates tilde-expansion tests under a fake HOME; tilde read-back uses process.env.HOME ?? os.homedir() for portability.

Files touched (19 files, +1227 / -43)

.github/workflows/bump-version.yml          |  13 ++
.github/workflows/publish.yml               |  29 ++-
docs/CONFIGURATION.md                       |   2 +
src/config/config.ts                        |  50 +++++-
src/config/models.ts                        |  50 +++++-
src/flow/audit-formatters.ts                |  20 ++-
src/flow/cycle-guard.ts                     |  35 ++-
src/flow/execute-single.ts                  |  45 ++++-
src/flow/executor.ts                        |   7 +-
src/index.ts                                |   9 +-
src/tools/trace.ts                          |  14 +-
src/types/flow.ts                           |  25 +--
tests/audit-formatters.test.ts              | 141 +++++++++++++++
tests/batch.test.ts                         |  16 +-
tests/config.test.ts                        | 268 ++++++++++++++++++++++++++-
tests/cycle-guard.test.ts                   | 158 +++++++++++++++
tests/execute-single-invalid-models.test.ts | 244 +++++++++++++++++++++++++
tests/trace-runtime.test.ts                 | 123 +++++++++++++
tests/types.test.ts                         |  21 +++

Commits

4ae1397 fix: address PR #35 review follow-ups in one coordinated pass
57ccff6 test: close cycle-guard Branch A gap + fix _clearSettingsCache models cache isolation
8255591 refactor: review fixes — inline usage guards, drop dead regex/interface, hoist deps
025f090 fix: close PR #35 review gaps
b9815db test(batch): isolate tilde expansion home writes
6299ea5 ci: harden manual release checkout
779bc26 fix: filter invalid/ambiguous candidates before effectivePrimary selection
1b46bc1 fix(config): address PR #35 review — size snapshots from first valid model
7e41f70 fix(flow): fail over on missing model errors (rebased on main)

Verification

  • npx tsc --noEmit — clean.
  • npx vitest run for the seven targeted suites — 286 / 286 tests pass (cycle-guard, config, audit-formatters, execute-single-invalid-models, trace-runtime, types, batch).
  • npx tsc (full build) — clean.

Review follow-up

Every open item from the post-merge PR #35 review is closed in 4ae1397. The bundled CI / test-isolation / telemetry hardening documented above is intentional scope for this PR.

Closes PR #35 review (memory learned_skill_experience / PR Review Gap Closure Workflow).

@PhucHai123 PhucHai123 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: REQUEST CHANGES

This PR addresses a real failure mode by adding model failover for provider 404/resource-not-found errors and by guarding missing usage fields. The new tests cover the main happy paths, and the implementation is directionally useful. It is not mergeable yet: the branch conflicts with current main, and the new models.json validation is applied inconsistently across flow entry points.

Build: npm run lint passed; npm run build passed
Tests: npm test passed with elevated filesystem permissions: 64 files / 1689 tests. The initial sandboxed run failed only on tests/batch.test.ts tilde-expansion write outside the workspace, which is environmental; new tests were added in tests/config.test.ts, tests/cycle-guard.test.ts, and tests/types.test.ts.


File 1: src/flow/execute-single.ts — PR merge integration

MAJOR — Branch conflicts with current main and overlaps sub-agent retry logic

Lines: 43-87

GitHub reports the PR as CONFLICTING, and local verification confirms it:

git merge-tree $(git merge-base main HEAD) main HEAD

The conflict is not cosmetic. Current main has sub-agent connection retry logic in executeSingleFlow, while this PR inserts the invalid-candidate guard and rewrites the surrounding model-attempt loop. A mechanical conflict resolution risks dropping either the retry behavior from main or the new bad-settings guard from this PR.

Impact: Blocking. The PR cannot be merged, and the most important changed file needs a deliberate rebase/merge.

Suggested fix:

  1. Rebase this branch onto current main.
  2. Keep the main retry loop intact.
  3. Insert the candidates.length === 0 && invalidCandidates.length > 0 guard before the retry loop computes attemptModels.
  4. Add or retain a test that proves invalid model settings do not enter the retry loop and do not call runFlow.

File 2: src/config/config.ts / src/config/models.tsresolveFlowModelCandidates()

MAJOR — models.json is treated as an authoritative allow-list for configured models

Lines: src/config/config.ts:525-528, src/config/models.ts:73-85

resolveFlowModelCandidates() now skips any provider/model value when hasConfiguredModel() returns false. That false is based only on the local models.json contents for an existing provider. If a user's local registry is stale but the provider actually supports the model, this rejects the model before runtime and can return a hard Bad settings result instead of allowing provider-side failover from a real resource_not_found response.

Impact: Potential production regression for users with stale or partial models.json files. The docs describe flowModelConfigs as user-configured strategies; they do not state that models.json is a strict model allow-list.

Suggested fix:

Make registry misses warning-only unless the project explicitly guarantees models.json is authoritative. A safer shape is:

const configured = hasConfiguredModel(normalized);
if (configured === false) {
  invalidCandidates.push(normalized);
  logWarn(`[pi-agent-flow] Model "${normalized}" is not present in models.json; trying it anyway.`);
}
candidates.push(normalized);

Then rely on the new resource_not_found_error / requested-resource failover in shouldFailover() to advance to the next candidate after an actual provider error. If the intent is a strict allow-list, document that contract in docs/CONFIGURATION.md and add tests for stale registry behavior.


File 3: src/tools/trace.tsresolveTraceRuntime()

MAJOR — Standalone trace bypasses the new all-invalid-candidates guard

Lines: 226-234

Normal flow execution handles all-invalid configured models in executeSingleFlow() at src/flow/execute-single.ts:54-84. The standalone trace path calls the same candidate resolver but only reads candidates[0]:

const { candidates } = resolveFlowModelCandidates(...);
const resolvedModel = candidates[0];

If every configured trace candidate is rejected, resolvedModel becomes undefined and runFlow() can fall back to flow.model or the inherited parent model. That means the same bad model configuration is reported as Bad settings for flow, but silently bypassed for trace.

Impact: Inconsistent runtime behavior and confusing diagnostics. A user can think a selected trace model strategy is being used while trace actually falls back to a different default model.

Suggested fix:

Have resolveTraceRuntime() consume invalidCandidates and return a failed trace result or throw a clear bad-settings error before runFlowWithLiveSession() starts:

const { candidates, invalidCandidates } = resolveFlowModelCandidates(...);
if (candidates.length === 0 && invalidCandidates.length > 0) {
  throw new Error(`Bad settings: all configured trace models are missing from models.json: ${invalidCandidates.join(", ")}`);
}

Also add a trace test with a models.json provider that lacks the configured trace model and assert that trace does not proceed with model: undefined.


File 4: src/flow/cycle-guard.tsshouldFailover()

PASS — Provider 404 and Kimi tool_call_id failover checks are appropriately scoped

Lines: 36-57

The new checks preserve the existing "permission" and "bad settings" no-failover behavior, keep generic invalid-tool errors from retrying, and add targeted handling for tool_call_id 400s plus provider resource-not-found messages. The added tests in tests/cycle-guard.test.ts cover both positive and negative cases.


File 5: src/types/flow.tsaggregateFlowUsage()

PASS — Missing usage fields no longer poison totals

Lines: 127-151

The defensive ?? 0 aggregation prevents missing cost or other usage fields from turning totals into NaN, and the new test in tests/types.test.ts covers the reported case. This is safe and mergeable after the branch-level blockers are resolved.


PR Conflicts/Blocking Gaps

MAJOR — Current branch is stale relative to main

Lines: repository-level

git log --left-right --cherry-pick main...HEAD shows this PR is a single commit behind a long sequence of main changes, including retry logic and tool-surface changes. Resolve the conflicts before merging so the failover fix lands on the current architecture rather than an older executor shape.

Summary

# Severity File Finding
1 Major src/flow/execute-single.ts:43 Branch conflicts with current main; invalid-model guard overlaps sub-agent retry loop
2 Major src/config/config.ts:525 Local models.json miss rejects configured models before provider runtime unless the registry is guaranteed authoritative
3 Major src/tools/trace.ts:226 Standalone trace ignores invalidCandidates and can silently fall back to another model
4 PASS src/flow/cycle-guard.ts:36 New resource-not-found and tool_call_id failover checks are covered and scoped
5 PASS src/types/flow.ts:127 Usage aggregation now tolerates missing fields without NaN totals

Recommendation: Request changes. Rebase onto current main, reconcile the new invalid-model handling with the retry loop, and make model validation behavior consistent for trace before this merges.

@PhucHai123 PhucHai123 force-pushed the fix/flow-404-failover-usage-guards branch from d5a0888 to d1a99c4 Compare June 29, 2026 12:02
@PhucHai123

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main and addressed all blockers raised in #4591520830.

Changes applied:

  • Rebased onto main, preserving sub-agent connection retries and settings-change emitter/cache.
  • resolveFlowModelCandidates() now returns invalidCandidates and warns (but still tries) models missing from models.json; fails fast only when every configured model is known-invalid.
  • executeSingleFlow() keeps the retry loop and adds the all-invalid guard before it.
  • resolveTraceRuntime() now throws a clear Bad settings error when all trace models are invalid.
  • hasConfiguredModel() accepts a cached registry so resolveFlowModelCandidates() reads models.json once per call.
  • shouldFailover() now handles resource_not_found_error / requested resource was not found and avoids blocking tool_call_id 400s behind the generic invalid tool filter.
  • aggregateFlowUsage() defensively coalesces missing usage fields.
  • Added regression tests in tests/cycle-guard.test.ts, tests/execute-single-invalid-models.test.ts, tests/trace-runtime.test.ts, and updated tests/config.test.ts / tests/types.test.ts.
  • Documented the models.json hint/warning behavior in docs/CONFIGURATION.md.

Verification:

  • npm run lint passes.
  • npm run build passes.
  • npm test -- --exclude tests/repro-flow-execute.test.ts passes: 84 files / 1926 tests. The excluded test is an unrelated pre-existing failure that depends on a local session file at /Users/__blitzzz/....

Ready for an independent reviewer to approve and merge.

@PhucHai123

Copy link
Copy Markdown
Collaborator Author

CI is now green. The failure was , which hard-codes a session file path at and fails in any environment without that file. I fixed it by skipping the test when the repro session file is absent (the author can still run it locally by setting ). Full suite now passes: 84 test files / 1926 tests passed, 1 skipped.

@PhucHai123 PhucHai123 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: APPROVE

The PR introduces robust safeguards and failovers for model resolution, missing models, and usage aggregation. It correctly handles provider resource-not-found errors, guards against missing usage/cost fields, and fails fast when every configured model for a flow is invalid. The test coverage is comprehensive and runs successfully.

Build: npm run build (successful)
Tests: npm test (successful, 1927/1927 tests passed)


File 1: src/config/config.tsresolveFlowModelCandidates()

MINOR — Sizing snapshots for known-missing primary models

Lines: 612–655

When a primary model is missing from models.json but a failover model is valid, resolveFlowModelCandidates still returns the missing model as primary.

Callers like resolveSnapshotMaxContextTokens() in src/index.ts use primary to determine the maximum context window for compressing parent context snapshots. Since the missing model returns undefined context window size, a large parent context might be sized/compressed incorrectly, only to fail over later to a smaller valid model.

Suggested Fix:
Consider making resolveFlowModelCandidates skip known-invalid candidates when selecting primary for context sizing, or expose an effectivePrimary field specifically for snapshot sizing.


File 2: src/flow/cycle-guard.tsshouldFailover()

PASS — Correctly identifies failover triggers and excludes non-retryable errors

Lines: 33–54

The function now correctly:

  1. Retries on tool_call_id 400 errors.
  2. Excludes bad settings errors from triggering failover.
  3. Identifies provider-side resource-not-found (resource_not_found_error or HTTP 404) errors to trigger failovers.

This is clean and behaves as intended.


File 3: src/flow/execute-single.tsexecuteSingleFlow()

PASS — Fails fast when all candidate models are invalid

Lines: 177–211

The PR correctly halts execution and returns a clear Bad settings error when all candidates are known to be invalid/missing, avoiding unnecessary calls to runFlow().


File 4: src/types/flow.tsaggregateFlowUsage()

PASS — Sturdier handling of missing usage/cost fields

Lines: 257–299

The changes correctly default missing fields (like cost, turns, toolCalls) to zero, preventing potential NaN values when aggregating usage from multiple flow results.


File 5: tests/ — Test Suites

PASS — Excellent regression and coverage tests

New unit tests in tests/cycle-guard.test.ts, tests/execute-single-invalid-models.test.ts, tests/trace-runtime.test.ts, and tests/types.test.ts provide comprehensive coverage for all new pathways. All tests pass successfully.


Summary

# Severity File Finding
1 Minor src/config/config.ts Context planning sizes snapshots for missing primary models instead of valid failover models.
2 Pass src/flow/cycle-guard.ts Correct failover triggers added for Kimi tool_call_id 400 and provider 404 errors.
3 Pass src/flow/execute-single.ts Fail fast strategy prevents execution when all candidates are missing.
4 Pass src/types/flow.ts Missing usage fields are defaulted to zero, avoiding NaN.
5 Pass tests/ Comprehensive test coverage added for all changes.

Recommendation: The PR is well-implemented, has comprehensive test coverage, and resolves critical provider-side errors. The primary model snapshot sizing issue is minor and can be addressed in a subsequent cleanup. I recommend approving and merging this PR.

@PhucHai123 PhucHai123 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: REQUEST CHANGES

The PR introduces robust safeguards and failovers for model resolution, missing models, and usage aggregation. It correctly handles provider resource-not-found errors, guards against missing usage/cost fields, and fails fast when every configured model for a flow is invalid. The test coverage is comprehensive and runs successfully. However, we should request changes due to the primary model snapshot sizing issue.

Build: npm run build (successful)
Tests: npm test (successful, 1927/1927 tests passed)


File 1: src/config/config.tsresolveFlowModelCandidates()

MINOR — Sizing snapshots for known-missing primary models

Lines: 612–655

When a primary model is missing from models.json but a failover model is valid, resolveFlowModelCandidates still returns the missing model as primary.

Callers like resolveSnapshotMaxContextTokens() in src/index.ts use primary to determine the maximum context window for compressing parent context snapshots. Since the missing model returns undefined context window size, a large parent context might be sized/compressed incorrectly, only to fail over later to a smaller valid model.

Suggested Fix:
Consider making resolveFlowModelCandidates skip known-invalid candidates when selecting primary for context sizing, or expose an effectivePrimary field specifically for snapshot sizing.


File 2: src/flow/cycle-guard.tsshouldFailover()

PASS — Correctly identifies failover triggers and excludes non-retryable errors

Lines: 33–54

The function now correctly:

  1. Retries on tool_call_id 400 errors.
  2. Excludes bad settings errors from triggering failover.
  3. Identifies provider-side resource-not-found (resource_not_found_error or HTTP 404) errors to trigger failovers.

This is clean and behaves as intended.


File 3: src/flow/execute-single.tsexecuteSingleFlow()

PASS — Fails fast when all candidate models are invalid

Lines: 177–211

The PR correctly halts execution and returns a clear Bad settings error when all candidates are known to be invalid/missing, avoiding unnecessary calls to runFlow().


File 4: src/types/flow.tsaggregateFlowUsage()

PASS — Sturdier handling of missing usage/cost fields

Lines: 257–299

The changes correctly default missing fields (like cost, turns, toolCalls) to zero, preventing potential NaN values when aggregating usage from multiple flow results.


File 5: tests/ — Test Suites

PASS — Excellent regression and coverage tests

New unit tests in tests/cycle-guard.test.ts, tests/execute-single-invalid-models.test.ts, tests/trace-runtime.test.ts, and tests/types.test.ts provide comprehensive coverage for all new pathways. All tests pass successfully.


Summary

# Severity File Finding
1 Minor src/config/config.ts Context planning sizes snapshots for missing primary models instead of valid failover models.
2 Pass src/flow/cycle-guard.ts Correct failover triggers added for Kimi tool_call_id 400 and provider 404 errors.
3 Pass src/flow/execute-single.ts Fail fast strategy prevents execution when all candidates are missing.
4 Pass src/types/flow.ts Missing usage fields are defaulted to zero, avoiding NaN.
5 Pass tests/ Comprehensive test coverage added for all changes.

Recommendation: Do not merge.

…model

- Add effectivePrimary to resolveFlowModelCandidates and use it for snapshot context sizing.
- Cache parsed models.json across calls (invalidateModelsJsonCache for tests/hot-reload).
- Add explicit TraceRuntimeResolution return type to resolveTraceRuntime.
- Pick the first valid trace model candidate when primary is missing.
- Add partial-valid trace runtime regression test.
- Fix whitespace typo in config test.
@PhucHai123

Copy link
Copy Markdown
Collaborator Author

Review: REQUEST CHANGES

This PR adds missing-model detection, failover on provider 404/tool-call errors, usage guards, and skips a local-session repro test when the fixture is absent. The main flow retry path is covered, but the new invalid-candidate signal is not used by non-execution callers, so mixed invalid-primary/valid-failover configs still size or select from the known-missing primary.

Build: npm run build on PR-head archive: pass. npm run lint: pass.
Tests: PR-focused tests pass: npx vitest run tests/config.test.ts tests/cycle-guard.test.ts tests/execute-single-invalid-models.test.ts tests/trace-runtime.test.ts tests/types.test.ts tests/repro-flow-execute.test.ts = 95 passed, 1 skipped. Full npm test after build has 1 unrelated sandbox/home write failure in pre-existing tests/batch.test.ts > tilde expansion > allows ~ path that resolves outside cwd for write.

File 1: src/config/config.ts / src/index.ts / src/tools/trace.ts / src/flow/audit-formatters.ts — model candidate selection

MAJOR — Known-missing primaries are still used for snapshot sizing and trace/audit selection

Lines: src/config/config.ts:612, src/index.ts:689, src/index.ts:700, src/index.ts:707, src/tools/trace.ts:232, src/tools/trace.ts:244, src/tools/trace.ts:245, src/flow/audit-formatters.ts:24, src/flow/audit-formatters.ts:31, src/flow/audit-formatters.ts:32

resolveFlowModelCandidates() now correctly returns invalidCandidates, and executeSingleFlow() only fails fast when every candidate is known missing. But several callers still consume primary or candidates[0] after validation. For a config like:

lite: {
  primary: "kimi/kimi-for-coding",      // missing from models.json
  failover: ["kimi/kimi-k2.7-code"],    // present with contextWindow
}

resolveSnapshotMaxContextTokens() uses the missing primary, so buildSnapshotWithCompression() receives undefined and does not size the fork snapshot to the valid failover model. resolveTraceRuntime() also resolves trace to the missing primary even though a known-valid failover exists. resolveAuditModel() has the same ghost/result sizing mismatch for audit-loop state.

Minimal fix:

const effectivePrimary = () => {
  const invalid = new Set(invalidCandidates);
  return candidates.find((candidate) => !invalid.has(candidate));
};

return {
  primary: candidates[0],
  candidates,
  invalidCandidates,
  effectivePrimary: effectivePrimary(),
};

Then use effectivePrimary in the non-execution sizing/selection paths:

const { effectivePrimary } = resolveFlowModelCandidates(...);
const tokens = resolveModelContextWindow(effectivePrimary);

For trace:

const { candidates, invalidCandidates, effectivePrimary } = resolveFlowModelCandidates(...);
if (invalidCandidates.length > 0 && candidates.length === invalidCandidates.length) throw ...
const resolvedModel = effectivePrimary;

Add one small regression test for primary-missing/failover-present behavior in trace runtime, and one for snapshot max-context selection if that helper is practical to test. No extra abstraction is needed beyond returning the already-computed effective candidate.

PASS — Flow execution fail-fast and failover behavior is covered

Lines: src/flow/execute-single.ts:66, src/flow/execute-single.ts:77, src/flow/cycle-guard.ts:36, src/flow/cycle-guard.ts:55

The executor avoids calling runFlow() when all candidates are known missing, preserves model failover for retryable provider errors, and the added tests cover missing-all, mixed-valid, tool-call-id, resource-not-found, and bad-settings behavior.

File 2: tests/repro-flow-execute.test.ts — repro fixture guard

PASS — Local-only repro test is now gated

Lines: tests/repro-flow-execute.test.ts:76

Skipping the repro when the local session file is absent is the minimal fix. It prevents CI/local clones without that private fixture from failing while keeping the repro runnable where the fixture exists.

PR Conflicts/Blocking Gaps

The blocking gap is the effective-candidate mismatch above. It conflicts with the PR's own stated behavior: a missing registry entry should not silently block valid failover models, but the trace/snapshot/audit sizing paths still effectively privilege the known-missing primary.

Summary

# Severity File Finding
1 MAJOR src/config/config.ts:612, src/index.ts:689, src/tools/trace.ts:232, src/flow/audit-formatters.ts:24 invalidCandidates is ignored by sizing/selection-only callers, so known-missing primaries still drive snapshot, trace, and audit model sizing when a valid failover exists.
2 PASS src/flow/execute-single.ts:66, src/flow/cycle-guard.ts:55 Flow fail-fast and provider-error failover paths are covered by focused tests.
3 PASS tests/repro-flow-execute.test.ts:76 Local repro test is gated when its session fixture is absent.

Recommendation: Request changes. Fix the candidate-selection root cause once in resolveFlowModelCandidates() and switch the non-execution callers to the effective candidate; that is the smallest fix and avoids per-caller special cases.

…ction

- trace.ts: replace manual invalidSet filtering with effectivePrimary from resolveFlowModelCandidates
- audit-formatters.ts: use effectivePrimary instead of candidates[0] to skip invalid/ambiguous entries
- Add regression tests for effectivePrimary selection in audit-formatters

Fixes models like gpt-5.4-pro being selected as primary when they're
marked invalid in models.json, even when valid alternatives exist.
@PhucHai123 PhucHai123 force-pushed the fix/flow-404-failover-usage-guards branch from 60478e9 to 779bc26 Compare June 30, 2026 17:40
- config: route resolveFlowModelCandidates through the cached
  readModelsJson reader and invalidate it on writeFlowSetting /
  writeGlobalFlowMode so a hot-edited models.json is picked up.
- cycle-guard: only treat bare 'requested resource was not found' as a
  provider 404 when paired with a 404 token or the JSON envelope, and
  keep the canonical 'resource_not_found_error' literal so benign
  stderr text cannot trigger failover. Adds 3 regression tests.
- executor: drop dead imports (resolveFlowModelCandidates,
  resolveModelContextWindow) that are no longer used in this file.
- ci: replace 'git checkout main && git reset --hard FETCH_HEAD' with
  'git checkout -B main FETCH_HEAD' so the release runner leaves a
  clear checkout state.
- config: dedupe the missing-model warning by emitting one summary
  per resolution instead of one logWarn per invalid candidate.
@PhucHai123 PhucHai123 requested a review from tuanhung303 July 1, 2026 07:44
@PhucHai123 PhucHai123 self-assigned this Jul 1, 2026
Closes every open review item from the post-merge PR #35 review:

cycle-guard (critical): replace the regressed 2-shape trigger with a
4-shape OR — canonical JSON envelope (`"type":"resource_not_found_error"`),
bare machine type marker (`resource_not_found_error`), English phrase +
literal 404 token, and 404 token + `/v\d+/...` API-path context. The new
shape closes the asymmetry with NON_RETRYABLE_CONNECTION_PATTERNS where a
bare 404 already denied connection retries, and keeps the negative space
(benign "requested resource was not found" in tool output or docs text
does NOT trigger). Six regression tests added across positive and
negative paths.

config (correctness): invert cache invalidation so only
writeFlowModelConfig clears _modelsJsonCache — settings.json writes do
not touch models.json, so writeFlowSetting and writeGlobalFlowMode no
longer cause wasted registry re-reads. Lazy-load the registry inside
resolveFlowModelCandidates: empty strategies (no flowModel /
cliTierOverride / strategy[tier] / fallbackModel) now skip the disk
read entirely and emit no spurious "Failed to read settings JSON" warn
in fresh tmpDir environments. Three regression tests added (cache
invalidation boundaries suite + empty-strategy no-warn test).

index.ts (correctness): wire invalidateModelsJsonCache() into
session_start next to invalidateSettingsCache() so long-running daemons
honor external models.json edits without restart.

execute-single (correctness): leave model undefined in the fail-fast
SingleResult so onFlowMetrics does not poison per-model telemetry
rollups. The missing-model list is preserved in stderr /
errorMessage for diagnostics. Tests assert both result.model and
metrics.model are undefined.

audit-formatters (correctness): detect requested → effective drift in
resolveAuditModel and emit a one-time logWarn so silent audit-model
swaps (stale models.json, provider rename) are operator-visible.
Positive + negative tests.

CI (correctness): rename concurrency group to release-bump-version,
add a 30-minute job timeout to cap a hung release at the queue, share
the same group + timeout with publish.yml, and drop publish.yml's
redundant tag-push trigger that produced deterministic npm 409s
against bump-version.yml's own publish step. Documented at the head of
publish.yml.

docs (correctness): CONFIGURATION.md documents both the coalesced
missing-model warning and the model: undefined contract on the
fail-fast path so downstream consumers / log scrapers have a written
contract.

Verification: tsc --noEmit clean, 286 / 286 tests pass across the
targeted suites (cycle-guard, config, audit-formatters,
execute-single-invalid-models, trace-runtime, types, batch).
@PhucHai123 PhucHai123 merged commit e155360 into main Jul 2, 2026
1 check passed
@PhucHai123 PhucHai123 deleted the fix/flow-404-failover-usage-guards branch July 2, 2026 16:13
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