fix(resume): narrow the credential refresh to secrets, and thread model_role/provider_preferences through delegate resume - #292
Merged
Conversation
added 3 commits
September 2, 2026 17:02
resume_sub_session re-applies live settings.yaml provider overrides onto a
resumed sub-session's PERSISTED mount plan, for one stated reason (see the
block's own comment at session_spawner.py): on-disk metadata has its secrets
redacted, so a resumed session would otherwise send `Bearer [REDACTED]`.
But the merge it used was the full one. The drop site, quoted at file:line:
session_spawner.py:1005-1010 (pre-fix numbering)
_live_provider_overrides = _live_settings.get_provider_overrides()
if _live_provider_overrides:
_refreshed_providers = _apply_provider_overrides(
merged_config["providers"], _live_provider_overrides
)
-> runtime/config.py:564 merged = merge_module_items(provider, override_map[key])
-> lib/merge_utils.py:149-152
if key == "config" and key in merged:
merged["config"] = deep_merge(merged["config"], value)
-> lib/merge_utils.py:64 "Deep merge two dicts, with overlay winning conflicts."
base = the child's persisted provider config (priority: 0, installed by
model_role/provider_preferences at spawn); overlay = the settings override
(priority: 14 for the promoted provider on the measured host). Overlay wins,
so the promotion was destroyed and the resumed leg silently re-resolved to
whatever sits at settings priority 0.
priority is not a secret. This is collateral damage from the metadata
redaction security fix.
MEASURED (model_performance-rc0, 2,078 session files, 22 GB, 12 capture roots):
- 66 of 778 delegate sessions contain a session:resume; 39 (59%) change
model across the boundary; 37/39 cheap -> expensive
- all 39 report basis="priority" on BOTH sides -- a wipe, not a fallback
- 0 of 179 ROOT resumes affected: a root plan has no promotion to lose
- 402 of 29,702 captured requests (1.35%); worst single run 25.0%
THE FIX
Narrow the resume-time provider override to the keys redact_secrets() actually
redacted, reusing amplifier_core's own SENSITIVE_KEYS so the two directions
can never drift apart. Identity keys (module, id) are carried through so the
override still matches its target; every other top-level key is dropped, so a
settings override cannot rewrite `source` at resume time either.
Scope: RESUME only. Root/fresh config assembly (resolve_bundle_config) still
merges overrides in full -- there settings ARE the intended source of truth and
there is no persisted child promotion to protect. The hook refresh in the same
block is deliberately NOT narrowed; the reasoning is recorded inline.
This also closes rc0 section 4.6, which it could only record as
INFERRED-NOT-CONFIRMED: merge_utils.py:152 merges every settings key, so
reasoning_effort was structurally exposed to the same wipe. The capture had
"high" on both sides and could not observe it; the new test uses differing
values and settles it.
Tests: tests/test_narrow_overrides_to_secrets.py (13, no API calls).
…ate resume path
The narrowing in the previous commit stops the promotion being DESTROYED.
This commit lets a resumed leg REBUILD it -- so it re-resolves against the
current provider set, and says so honestly when it cannot.
THE GAP, IN ONE GREP
apply_provider_preferences_with_resolution appeared EXACTLY ONCE in
session_spawner.py -- inside spawn_sub_session. It was not reachable from the
resume path at all. The narrowing continued the whole way down (pre-fix
numbering, rc0 section 2.2):
hop spawn resume
tool-delegate call site __init__.py:1509 prefs+role __init__.py:1444 neither
capability invocation __init__.py:1771 spawn_fn __init__.py:2084 resume_fn
app-cli capability :715,730 (…prefs…) :736,1253 -- 2 args
app-cli implementation :231,241 spawn_sub_session :923 resume_sub_session
promotion applied? :417-421 never
WHAT THIS ADDS
- resume_sub_session(..., provider_preferences=None, model_role=None)
- both resume capability closures in session_spawner.py and the one in
session_runner.py gain the same two optional keyword arguments, matching
their child_spawn_capability siblings. Optional, so a caller still
invoking (sub_session_id, instruction) is unaffected.
- the resume path now applies apply_provider_preferences_with_resolution
exactly as spawn_sub_session does.
RECOVERY SOURCES -- why this reaches sessions nobody re-plumbed
Preference precedence is: threaded by the caller > the persisted agent
overlay > the persisted mount plan's own copy. The last two matter because
the rc0 capture showed both were still sitting in the resumed session's
config, "simply never consulted again". Recovering them means an existing
caller that has not yet been taught to thread preferences keeps its promotion
anyway; the threaded argument is what a caller gains when it is.
FALLBACK IS NAMED, NEVER SILENT
Per the rc0 acceptance criteria: when no preferred provider is mounted, the
resume emits provider:fallback carrying reason, the requested preferences,
where they came from, and the provider/model the leg actually landed on --
instead of silently re-resolving by settings priority. Promotion success is
verified from the OUTCOME (a preferred provider at priority 0), not from the
apply call's return value, so it stays honest across foundation versions.
model_role is written onto the resumed config so the leg's routing hook
resolves the same role the spawn leg was given.
NOT IN THIS REPO: the matching change in microsoft/amplifier-foundation
(tool-delegate's _resume_existing_session at __init__.py:1997 and its call
site at :1444) is what makes the caller pass these through. The signatures
here are additive and backward-compatible precisely so the two can land
independently, and the recovery sources above mean the defect is fixed
either way.
Tests: tests/test_resume_preserves_provider_promotion.py (8, no API calls).
Fail-before on the pre-fix tree: 7 failed / 1 passed, the headline assertion
reproducing the capture exactly -- `assert 14 == 0`, 14 being the promoted
provider's settings priority on the measured host. The 1 pre-existing pass is
the negative control (a plan with no promotion is byte-identical after resume),
which must pass on both sides.
Lane bookkeeping for the two fix commits above: the file:line sites rc0 identified, the fail-before/pass-after evidence, the root-resume and credential-refresh verification, decisions taken without escalation, and what remains open. Kept as its own commit so the two fix commits stay clean and this is trivially droppable before merge.
Brian Krabach (bkrabach)
marked this pull request as ready for review
September 3, 2026 00:19
Collaborator
Author
Merge-queue verification — PASS, merging with
|
| Gate | Method | Result |
|---|---|---|
Fail-before/pass-after: resumed delegate retains spawn-time model_role |
Reverted just the 3 non-test source files (runtime/config.py, session_runner.py, session_spawner.py) to main's version, kept the new behavioral test file, ran it → 7 failed, 1 passed (matches PR's claim exactly); test_resumed_leg_keeps_its_model_role_promotion fails with assert 14 == 0 (byte-identical to the PR's quoted failure). Restored PR files (verified git diff pr-292 empty afterward) → 21/21 passed |
PASS |
| Credential refresh still refreshes secrets | Pre-existing test_resume_credential_refresh.py (5) + test_resume_redaction_guard.py (6) pass unmodified — 11 passed, both files untouched by this diff (empty git diff --stat on them). Note: PR body states "15" (5+10); actual collected count is 11 (5+6) — a miscount in the DONE-NOTE prose, not a behavioral discrepancy; both suites are pre-existing, untouched, and green. New test_credentials_are_still_refreshed + test_promotion_survives_with_no_recoverable_preferences (asserts api_key == "sk-live-luna" alongside priority == 0) pass |
PASS (with a noted numeric discrepancy in the PR's own bookkeeping, not a correctness issue) |
| ROOT-session resume unaffected | grep narrow_overrides_to_secrets( → exactly 1 call site, inside resume_sub_session; grep session_spawner amplifier_app_cli/commands/session.py → 0 hits (root resume path never reaches this code at all) |
PASS |
| Default otherwise byte-identical | New args (provider_preferences=None, model_role=None) are additive/optional on both resume_sub_session and resume_capability; test_no_preferences_leaves_the_plan_byte_identical passes (asserts config["providers"] == _persisted_child_providers(), no provider:fallback emitted) |
PASS |
| Full suite green | uv run pytest -q → 1594 passed, 1 skipped, 13 deselected, 1 xfailed (matches PR's reported number exactly) |
PASS |
| CI (all legs) | gh pr checks 292 → ubuntu×2, macos×2, integration×2, cla all pass; windows×2 (py3.11/3.12) fail on tests/test_timedout_session_resumable.py (2 tests, unrelated file, not touched by this PR). Verified pre-existing: main@0d93352 (this PR's own base, and the tip of main right now) fails the identical two tests on the identical two Windows legs in its own push-CI run (gh run view 33685676024) — introduced by the immediately-prior PR #291-equivalent commit, not by this one |
PASS (failure is pre-existing on main, not introduced by this diff — flagging per the "if conflicting, don't paper over it" spirit even though it's CI-flake territory, not a merge conflict) |
| Diff touches only what the title says | git diff main...pr-292 --stat → runtime/config.py, session_runner.py, session_spawner.py, docs/lanes/n1i-resume-thread-role/DONE-NOTE.md, 2 new test files. All in scope for "narrow credential refresh to secrets" + "thread model_role/provider_preferences through resume" |
PASS |
| Lint | uv run ruff check . → 14 errors, identical before (main) and after (pr-292) — none introduced |
PASS |
| No unvalidated performance claim | All quantitative claims (39/66 delegate resumes, 37 cheap→expensive, 402/29,702 requests, 0/179 root resumes) are cited from rc0's prior CONFIRMED-ON-WIRE-EVIDENCE findings, not re-measured or asserted as new by this lane. The one unmeasured figure (dollar delta of the 402 drifted requests) is explicitly labeled "NOT COMPUTED" rather than estimated or implied | PASS — honest |
Concurrency check: lane adq is reported live on this repo. main at verification time is still 0d93352 (the PR's own base) — no divergence, no conflict to reconcile.
Surprising/notable:
- The PR's own DONE-NOTE miscounts
test_resume_redaction_guard.pyas having 10 tests; it actually has 6 (file is untouched by this PR either way — a pre-existing suite, verified green). Noted for the record; does not change any gate outcome. - The two Windows CI failures are pre-existing on
mainitself (confirmed viamain's own push-CI run), inherited from the immediately-prior commit0d93352(fix(spawn): checkpoint sub-session transcripts...), not introduced by this PR. Full suite is green on ubuntu/macos/integration/cla; this scratch-clone verification (Linux) also shows 100% green.
All gates pass. Merging via gh pr merge --squash --admin (required-review ruleset bypass disclosed here, as instructed — I am the PR author and no other human reviewer is available in this workflow).
This was referenced Sep 3, 2026
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.
Implements the root cause diagnosed in
model_performance-rc0(CONFIRMED ON WIRE EVIDENCE, n=39 affected delegate sessions across 24 runs, 0/179 root sessions affected). Two defects, two commits. Spend: $0.00 — no API calls in any new test.DONE-NOTE —
model_performance-n1iFIX: narrow the resume credential refresh to secrets, and thread
model_role/provider_preferencesthrough the delegate resume pathLane
n1i-resume-thread-role· repomicrosoft/amplifier-app-cli·branch
lane/n1i-resume-thread-role· 2026-09-02 · spend $0.00Implements the root cause diagnosed in
model_performance-rc0(
ai-notes/w3-rc0-resume-role-loss/FINDINGS.md). That diagnosis was CONFIRMEDON WIRE EVIDENCE and was not re-litigated here; this lane implemented it.
1. DELIVERABLES
lane/n1i-resume-thread-role, two fixes as separate commits, tests greenmodel_rolepromotionCommits:
2. COMMIT A — the DROP SITE, narrowed to secrets
amplifier_app_cli/runtime/config.py,amplifier_app_cli/session_spawner.py,tests/test_narrow_overrides_to_secrets.pyThe sites rc0 identified, quoted
Pre-fix line numbers are rc0's, taken against
openai-evals-team-ci/amplifier-app-cli @ ed89a9f. This lane's worktree is at0d93352, a later commit that shifted the block down ~200 lines; currentnumbers are given alongside.
session_spawner.py:1005-1010(here:1214-1219pre-fix), insideresume_sub_session:runtime/config.py:564—_apply_provider_overrides:lib/merge_utils.py:149-152— where the promotion is actually lost:lib/merge_utils.py:64-65:base= the child's persisted provider config (priority: 0, installed atspawn by
spawn_utils.py:772-773);overlay= the settings override(
priority: 14for luna on the measured host). Overlay wins.The enclosing comment block (
session_spawner.py:971-991) scopes this refreshto secrets.
priorityis not a secret — collateral damage from themetadata-redaction security fix.
The change
New
narrow_overrides_to_secrets()inruntime/config.pyreduces a settingsoverride to the keys
redact_secrets()actually redacted, reusingamplifier_core.utils.truncate.SENSITIVE_KEYSso the two directions cannever drift apart — if redaction learns a new secret key, the refresh learns
it too, with no second list to maintain.
Pruning rules, and why each is what it is:
recurse, keeping the key only if something secret survives beneath it
(covers
config.auth.token).deep_mergereplaces lists rather than merging them, so a partiallypruned list would silently truncate the merged result. All-or-nothing is the
only safe choice.
moduleandidcarried through so the override stillmatches its target; every other top-level key dropped, so a settings
override cannot rewrite
sourceat resume time either.Scope, deliberately narrow: applied at the RESUME call site only. Root and
fresh config assembly (
resolve_bundle_config) still merges overrides infull — there settings are the intended source of truth and there is no
persisted child promotion to protect. Verified:
narrow_overrides_to_secretshas exactly one call site (§4).
rc0 §4.6 — the INFERRED-NOT-CONFIRMED sub-claim, now settled
rc0 could not observe
reasoning_effortdrift because the capture hadhighon both sides.
merge_utils.py:152merges every settings key, so the driftwas structurally possible but unobservable.
test_per_candidate_config_keys_surviveuses a preference effort ofmediumagainst a settings effort of
highand asserts the child's own value wins.Verdict upgraded: CONFIRMED, and fixed.
3. COMMIT B — the THREADING
amplifier_app_cli/session_spawner.py,amplifier_app_cli/session_runner.py,tests/test_resume_preserves_provider_promotion.pyThe gap, as rc0 §2.2 mapped it
__init__.py:1509— prefs + role__init__.py:1444— neither__init__.py:1771spawn_fn(… provider_preferences=…)__init__.py:2084resume_fn(sub_session_id=…, instruction=…)session_spawner.py:715,730session_spawner.py:736,1253— 2 argssession_spawner.py:231,241spawn_sub_session(… provider_preferences …)session_spawner.py:923-926resume_sub_session(sub_session_id, instruction, parent_session)session_spawner.py:417-421→apply_provider_preferences_with_resolutionThe decisive grep:
apply_provider_preferences_with_resolutionappearedexactly once in
session_spawner.py, insidespawn_sub_session.The change
resume_sub_session(..., provider_preferences=None, model_role=None)child_resume_capabilityclosures (session_spawner.py) andresume_capability(session_runner.py:553) gain the same two optionalkeyword arguments, matching their
child_spawn_capabilitysiblingsapply_provider_preferences_with_resolutionexactly as
spawn_sub_sessiondoesPreference precedence: threaded by the caller > persisted agent overlay >
persisted mount plan's own copy.
The two recovery sources are the load-bearing design decision. rc0 §3 recorded
that both
provider_preferencesandmodel_rolewere still present in theresumed session's config and simply never consulted again. Recovering them
means the fix reaches existing sessions and existing callers without the
foundation-side change; the threaded argument is what a caller gains once it is
taught to pass them.
Fallback is named, never silent. Per the rc0 acceptance criteria: when no
preferred provider is mounted, resume emits
provider:fallbackcarryingreason, the requested preferences,preferences_source, and theprovider/model the leg actually landed on. Promotion success is verified from
the outcome (a preferred provider sitting at
priority: 0), not from theapply call's return value, so the check stays honest across foundation
versions.
NOT IN THIS REPO — stated plainly
The matching change in
microsoft/amplifier-foundation(
modules/tool-delegate/.../__init__.py:1997_resume_existing_session, itscall site at
:1444, andresume_fnat:2084) is what makes the callerpass these through. This lane owns
amplifier-app-clionly, so that repo isuntouched. The signatures added here are additive and backward-compatible
precisely so the two can land independently — and the recovery sources above
mean the measured defect is fixed either way.
4. DELIVERABLE 3 — root resume and credential refresh are unaffected
Verification script output, run against both commits:
Root-session resume — how it was verified.
resume_sub_sessionhas exactlyone importer in the whole package:
session_runner.py:519, insideregister_session_spawning, which registers it as thesession.resumecapability — that is root-resumes-a-SUB-session, not root resumes itself.
Root-session resume runs through
commands/session.py::_prepare_resume_context→
resolve_config→resolve_bundle_config, which importssession_spawnerzero times (check 4) and whose override merging this change does not touch
(check 1: the narrowing has a single call site, inside
resume_sub_session).This matches rc0's own strongest negative control — 0 of 179 root resumes
affected — from the opposite direction: rc0 measured that roots were never
hit; this shows structurally that they still cannot be.
Credential refresh — how it was verified. The pre-existing suites
test_resume_credential_refresh.py(5) andtest_resume_redaction_guard.py(10) pass unmodified. Two new tests assert the refresh still does its job on
the narrowed path:
test_credentials_are_still_refreshed(unit) andtest_promotion_survives_with_no_recoverable_preferences(through the realresume_sub_session, assertingapi_key == "sk-live-luna"alongsidepriority == 0).Default behaviour otherwise byte-identical.
test_no_preferences_leaves_the_plan_byte_identicalresumes a plan carrying nopromotion and asserts
config["providers"] == _persisted_child_providers()with no
provider:fallbackemitted. It is the one test that passes on bothsides of the fix — the negative control.
5. FAIL-BEFORE / PASS-AFTER
Fail-before, both fixes reverted (
git stash push amplifier_app_cli/, run,pop) — genuine assertion failures, not a collection error, because the
behavioural module deliberately imports no symbol introduced by either fix:
14is the promoted provider's byte-exact settings priority from themeasured host (
~/.amplifier/settings.yamldeclaresluna: priority: 14).The unit test reproduces the wire signature exactly.
The 1 pre-existing pass is the negative control described above.
Fail-before for commit B alone (commit A applied, B reverted):
4 failed, 4 passed. The 4 that already pass are the ones commit A alone fixes — which iswhat the item predicted ("This alone resolves the observed defect"). The 4 that
still fail are commit B's distinct value: explicit preference threading,
preference-config re-assertion,
model_rolethreading, and theprovider:fallbackevent.Pass-after: 21/21 across both new modules; 1594 passed, 1 skipped, 1
xfailed for the full suite. No API calls in any new test; the whole suite
runs in ~8 s.
Lint:
ruff checkreports 14 errors before and 14 after — allpre-existing, none introduced.
ruff formatapplied to the touched files.6. DECISIONS TAKEN WITHOUT ESCALATION
Per the lane rules ("no waiting on any human decision: choose, record, continue"):
"guard against the same class in the sibling paths". For providers the guard
is the code change; for hooks it is a documented boundary, and the reasoning
is recorded inline at the site. Two reasons: (a) nothing in a hook entry
carries per-session resolution state — the provider wipe mattered because
config.prioritydecides which model a leg runs on, and a hook has noanalogue; (b)
get_notification_hook_overrides()legitimately appendshooks absent from the persisted plan, so narrowing to secrets would append
them stripped of
enabled/topic, breaking notifications on resumedsub-sessions to fix a defect nobody has observed. The same over-reach is
structurally possible for a hook whose config an agent overlay customised;
that needs its own evidence, not a speculative change. Flagged as a known
open edge rather than silently fixed or silently ignored.
_apply_provider_overrides/_apply_hook_overrides/ the tool overridemerge themselves are unchanged. Root config assembly depends on their full
merge semantics. Narrowing them at source would have changed root behaviour
to fix a resume-only defect.
provider:fallback, matching theprovider:namespace ofprovider:resolve/provider:retry/provider:errorinamplifier_core.events. No existing constant covered"the pin could not be honoured".
docs/lanes/n1i-resume-thread-role/as its own third commit, so the two fix commits stay clean and the lane
bookkeeping is trivially droppable before any merge. (First choice was to
keep it outside the repo entirely; the lane's write sandbox is the repo, and
the deliverable requires it committed under this lane's own directory.)
test_narrow_overrides_to_secrets.py; the behavioural module imports nonew symbol, so a fail-before run produces real assertion failures rather than
an
ImportErrorat collection. The first draft did not do this and itsfail-before run was a worthless collection error — corrected before commit.
7. SPEND
$0.00, against a $0 authority. No API calls, no DTU, no containers, no
infrastructure created — so nothing to register in the infra ledger and nothing
to tear down. Every test runs offline against mocked sessions.
8. WHAT REMAINS OPEN
microsoft/amplifier-foundationtool-delegate is untouched (§3). Untilit threads
provider_preferences/raw_model_roleintoresume_fn, thepromotion is rebuilt from the persisted preference rather than a freshly
routed one. Functionally equivalent for the measured defect; it does mean a
routing-matrix change between two legs of the same delegate is not picked up
mid-session.
left with a named comment instead of a speculative fix.
amplifier-bundle-routing-matrix'srole_pin.py(rc0 §5) should stay asdefense-in-depth. This fix is upstream of it; it is now belt-and-braces
rather than the only guard.
this as NOT COMPUTED and this lane did not change that — it needs per-request
usage joined to per-model pricing, and would not have changed any decision
here.
00-what-we-know.md§2c's "symmetric confounder" justification shouldstill be amended per rc0 §4.5 — this bias is asymmetric and exaggerates the
measured cost spread between cells. That is a docs change in the evals repo,
outside this lane's paths.