fix(framework): close the silent failures left by the framework_agent rename - #1122
Open
zengleixin-amd wants to merge 25 commits into
Open
fix(framework): close the silent failures left by the framework_agent rename#1122zengleixin-amd wants to merge 25 commits into
zengleixin-amd wants to merge 25 commits into
Conversation
…registry The FRAMEWORK pump hardcoded its lane list and never passed lease_ttl_sec, so every framework_agent task row was created with TTL 0. The dispatcher turns that into a 60s lane lease for an action whose declared p50 cost is 12 minutes, letting another task legitimately take server_lifecycle, workspace_mutation or benchmark_lane mid-run. TTL 0 also made the watchdog skip reclamation of orphaned framework tasks entirely. Resolve both values through _registry_lanes_ttl, the helper the prelude, explore, proposals and intent_router enqueue paths already use, so the action metadata stays the single source of truth instead of being duplicated at the call site. The regression test derives its expectations from the registry rather than hardcoding 3600, and also asserts the TTL is non-zero and that no declared lane is dropped, so a silent empty resolution cannot pass. Two framework test stubs gain real registry wiring because their materialize path reaches the same enqueue. Co-authored-by: Cursor <cursoragent@cursor.com>
Resume compared every recorded KEEP against the optimization stack using the task kind and a locally re-derived variant name. For framework KEEPs both halves of that key were wrong, so a KEEP that had landed correctly was reported as an orphan on every resume, permanently polluting the resume report with a warning and a medium observation. The action half was wrong because framework KEEPs are stacked under the ``framework`` attribution family label, which is what phase_breakdown publishes, while the event log records the ``framework_agent`` task kind. Name the label once as a module constant and translate at the single point where the two vocabularies meet. The variant half was wrong because the promote path decorated the stack entry name with a ``framework:`` prefix that nothing consumes, and passed the undecorated key in a ``variant_name`` field that _lift_to_current_best never reads. Drop both and let the canonical candidate key stand on its own, and have the reconciliation derive the same key through _framework_candidate_key instead of re-implementing its precedence, which had also silently omitted the ``ref`` fallback. A KEEP that is genuinely missing from the stack still alerts; a dedicated test pins that, so the fix cannot degrade into blanket suppression. Co-authored-by: Cursor <cursoragent@cursor.com>
Two classes of defect in this area produce no error at runtime, only a wrong-looking-but-plausible result, which is why both survived review. An action the Coordinator enqueues itself is never proposed by an agent, so a missing executor shows up as a silently failed task rather than a startup error. Assert instead that every Coordinator-internal action resolves to a registered executor, and that no registration carries a name absent from the action catalogue. Both directions read their expectations off the production vocabularies, so adding an action cannot leave the guard behind, and a third test pins the only two conditions under which a registration may legitimately be missing, so that exception set cannot quietly grow. The framework gain label is written in one module and matched independently in two collectors. Feed the writer's own constant through both readers, with no source_phase on the entry, since that field lets one collector resolve the owner without consulting the action label and would mask the very mismatch being guarded. Renaming one side alone now fails here instead of routing FRAMEWORK gain into unattributed. Both guards were validated by injecting the defects they describe: the stale executor key reproduces the missing and phantom keys, and renaming the label on the writer alone drops framework_pct_of_total to zero. Co-authored-by: Cursor <cursoragent@cursor.com>
… read PolicyGate denies the action it is actually handed, and interpolates that real name into the denial reason, but the accompanying hint still listed the pre-rename ``framework``. The orchestration prompt had the same stale name in the sentence telling the model which actions are Coordinator- managed, and omitted conc_sweep entirely. So an operator grepping logs for the name in the hint finds nothing, and a model reading the prompt can conclude framework_agent is fair game, spending a tick on a proposal that comes back as phase_incompatible. Neither gate logic nor prompt structure changes; only the names in the prose. Two guards read the expectation off COORDINATOR_INTERNAL_ACTIONS, so this text cannot drift again and a newly added internal action cannot be left out of either surface. Co-authored-by: Cursor <cursoragent@cursor.com>
The phase-budget flags default to None and fall through to DEFAULT_PHASE_BUDGET_PCT, so their help text is the only place a user can read the real share and nothing recomputes it. The KERNEL_AGENT and SWEEP shares had been retuned to 0.35 and 0.05 without the help following, so --help quoted 0.28 and 0.12. The phase_breakdown TypedDict declared a ``kernel`` bucket while the collector has always emitted ``kernel_agent``. Reading the published document through the declared shape therefore found that section empty. Only the annotation moves; the emitted JSON is unchanged, and no code indexed the old key. Both guards derive their expectation from the producing side, and the parser guard walks into the optimize subparser and refuses to pass on an empty scan, since an earlier draft collected nothing from the top-level parser and would otherwise have compared two empty sets. Co-authored-by: Cursor <cursoragent@cursor.com>
The workspace comment still pointed at runs/framework while the code writes runs/framework_agent, and a docstring still called the Coordinator-internal action framework. Two of these are not merely comments. The action description is loaded by the registry and rendered into the prompt, and it called the thing being applied an "upstream framework" rather than an upstream PR diff, which is the one place a reader could form the wrong idea about what the executor does. The params_schema pointed at $INFERENCE_OPTIMIZER_FRAMEWORK, which does not exist anywhere; the executor reads $FRAMEWORK. The kb.py docstring claimed installs always export FRAMEWORK_AGENT_ROOT so the third resolution step wins. A census of the development host found none of the three variables set anywhere, no .env file setting them, and no lessons.jsonl on disk, so orchestrator runs reach the fourth step and resolve to a directory that does not exist. The docstring now states that plainly instead of asserting the opposite; the resolver itself needs a single owner shared with kb_writeback, which is a separate change. Also drops a duplicate conc_sweep from INTERNAL_ONLY_ACTION_NAMES. The frozenset deduplicated it, so the resolved set is unchanged, but the literal read as five names when it only ever held four. Co-authored-by: Cursor <cursoragent@cursor.com>
Both reference documents told the reader the LLM never proposes the `framework` action. The runtime denies `framework_agent`, and per CLAUDE.md these files are agent-facing contracts rather than background prose, so the wrong name is read back by the same models the sentence is meant to constrain. Only the action name changes. Every other bare `framework` in these files refers to the inference framework, the attribution family, or a gap layer, and stays as it is. Co-authored-by: Cursor <cursoragent@cursor.com>
The orchestrator appended PR outcomes under the workspace while the fa reader resolved a packaged path that exists in no install, so the ledger always read back empty and every session re-proposed PRs it had already tried and rejected. Nothing raised, because an absent ledger is also what a cold start looks like. The two sides resolved the path independently, which is the actual defect. kb.mutable_kb_root is now the one owner and kb_writeback delegates to it, so a deployment that redirects the KB redirects both halves. A separate packaged_kb_root names the read-only seed tree shipped in the wheel, which is a different data class: an installed package may be read-only and an upgrade would overwrite anything written there. Both roots resolve per call. The writer used to resolve at import, which made it deaf to anything the process set up afterwards and forced tests to patch a cached constant instead of exercising the real lookup; those five call sites now redirect through the supported override, so they cover the path an operator would actually take. read_pr_ledger still returns an empty list for a missing file. That is a genuine cold start, not a misconfiguration, and is not the fallback this change is removing. The reader's remaining override chain is removed next, together with the tests that pin it. Co-authored-by: Cursor <cursoragent@cursor.com>
The module map is seed data built into the wheel, but it was resolved through the writable KB partition. Once that partition moved to the operator workspace the map stopped being readable at all, so a cross-framework port silently proceeded with no module mapping. Point it at the packaged root instead, which is where the file actually ships. Nothing writes the map at runtime, so the writable partition was never the right place to look for it. Verified on a deployment-shaped environment with no KB variables set: the map resolved to a non-existent workspace path before, and to the real packaged file after. Co-authored-by: Cursor <cursoragent@cursor.com>
FRAMEWORK_AGENT_KB_DIR moved the reader and nothing else, so setting it put reads and writes in different places. It could not be configured through a supported channel either: the dotenv allowlist does not carry it, so it only ever took effect for a process that exported it by hand. A run that still sets it now fails with a message naming the replacement, rather than being silently redirected to a location the writer will not use. FRAMEWORK_AGENT_ROOT is simply no longer consulted for the KB, and does not raise. It means "where this skill is installed", the main installer assigns it and setup.py scrubs it, so treating its presence as a KB misconfiguration would be wrong. It never reached the reader in practice anyway, because nothing exports it. What is left is one override and one default, with no chain to walk. Co-authored-by: Cursor <cursoragent@cursor.com>
The reference schema, the skill card and the agent card all documented a four-step resolution order whose first and third steps no longer exist and whose last step was described as a path that never resolves to a real KB. They also presented the KB as one tree. Describe the two roots instead, and say which data belongs in each: the mutable partition a session reads and writes, shared with the orchestrator's writeback, and the read-only seed shipped in the wheel. The withdrawn override keeps one mention, so an operator who hits the start-up error can find out why it went away. Co-authored-by: Cursor <cursoragent@cursor.com>
The FRAMEWORK enqueue now resolves its lanes and lease TTL through the registry, so any stub that reaches that call needs a registry to resolve against. This one binds the enqueue helper but did not exercise it, so it was left alone at the time; upstream's eval-policy tests now drive the enqueue through it, and the stub raises AttributeError before reaching the assertion under test. Wire it the same way as the other two framework stubs, so the lookup that runs in production is the one the test exercises. Co-authored-by: Cursor <cursoragent@cursor.com>
framework_agent_critic_decisions had two readers and no writer. The synchronous gate that used to fill it was replaced by the async proposal channel, and that change was reviewed as "no functional loss" because it covered the resume-cache use; the priors reader was left pointing at a list nothing populates any more. The working-memory reader is worse off still: it was written four days after the writer was removed, so its learnings have been empty since the day it landed, and the candidate ranker has never once seen why the Critic rejected something. The rejection reason was never actually lost. The gate stamps a critic_denied progress row carrying the rationale, and that ledger has a real writer. Point both readers at it and delete the field. The priors payload drops recent_decisions rather than rebuilding it from the same rows the outcomes already carry; the outcomes gain the rationale instead, so the Critic gets the argument behind each prior verdict rather than a key that was always empty. The status string is now a named constant shared by the writer and both readers, since three separate literals is how this class of defect starts. Co-authored-by: Cursor <cursoragent@cursor.com>
…always was framework_max_candidates sat in SharedState in front of DEFAULT_FRAMEWORK_MAX_CANDIDATES with no writer, no CLI flag and no environment variable, and PolicyGate listed it among the fields only the Coordinator may mutate. The comment beside that lock said "set once", but nothing ever set it, so the lock protected a value that could not change and discovery always asked for eight. Read the constant directly and drop the field, its gate entry and the robustness envelope mirror. The test that appeared to prove the cap was configurable is inverted into a guard: setting a same-named attribute must not move what discovery asks for, so the knob cannot quietly reappear without a real writer behind it. If a per-repo cap is wanted later it needs a CLI flag, a range check and a persistence story, none of which existed here. Co-authored-by: Cursor <cursoragent@cursor.com>
The script ran `pip install -e` against its own parent directory, which has no pyproject.toml, so it failed for anyone who followed the README. Nothing called it: no CI job, no other script, and the main installer had already been changed to stop chaining to it, since the fa CLI ships with the distribution. The env.sh it generated was never sourced by anything either. Repairing it would mean maintaining a second install path with no test behind it, for a CLI the main install already provides, so remove it and point the README at the repo-root install. Two things had to move with it. The package-data glob that matched it is now empty, and packaging lint fails on globs that match nothing, which is how a silently-dropped asset would otherwise be caught. The .gitignore entry still used the pre-src-layout path and never matched the generated file anyway. Co-authored-by: Cursor <cursoragent@cursor.com>
from_dict filters to known dataclass fields before any migration runs, so a state.json written before the framework_agent rename lost every FRAMEWORK key on load. Resume then reported an empty phase: already benchmarked PRs were queued again, and a persisted --no-framework-agent came back enabled. It returned a plausible default instead of failing, which is why the rename shipped without anyone noticing. Add a v5 step in the shape of the v4 enablement one: read the old spellings off raw, since the filter has already dropped them, and let the current spelling win when a half-migrated state carries both. Ten fields were renamed, not the eight named in the audit note, which missed framework_pr_phase_done and framework_pr_discover_failures. Eight are migrated here; the other two are the fields since removed for having no writer, so there is nothing to migrate them into. A guard asserts every target is still a real field and no legacy name has come back, because a stale table would drop data exactly as silently as the missing migration did. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolving it to <workspace>/kb put it in the recipe KB's legacy root, the directory cli/kb.py knows as _LEGACY_WORKSPACE_KB_ROOT and drains on first run through the recipe migration. Any workspace carrying pre-migration recipe trees therefore had them enumerated as framework domains, because list_domains reports every directory under this root as one. Move to <workspace>/framework-kb. Reader and writer already share one resolver, so a single line moves both, and nothing else changes: an explicit INFERENCE_OPTIMIZER_FA_KB_PATH still wins, and the packaged seed root is untouched. Nothing needs migrating. A census of the development host found no lessons.jsonl anywhere, and no document has ever named the framework KB path, so no deployment can be relying on the old location. The guard asserts the two KBs never resolve to the same root, or to one inside the other, which is the shape the collision would take if either side moved again. Co-authored-by: Cursor <cursoragent@cursor.com>
Four test stubs still declared framework_agent_critic_decisions or framework_max_candidates on their own state doubles, and _CRITIC_PRIORS_DECISION_TAIL survived the removal of the only code that read it, copied into two of those stubs along the way. None of it affected behaviour; it just left the impression that the fields are still part of the surface. The one remaining mention is deliberate: the guard that sets framework_max_candidates and asserts discovery ignores it. Co-authored-by: Cursor <cursoragent@cursor.com>
…ry the ledger over Three defects this branch introduced around the framework KB, all of the kind it exists to remove. Rejecting the withdrawn FRAMEWORK_AGENT_KB_DIR from _resolve_kb_root() put the error where nobody could act on it. Read paths treat the KB as advisory and swallow their own failures, so on a deployment that still exported the variable the RuntimeError landed in _config_lever_known_bad's except clause, the accuracy gate answered "not known bad" for every config, and a lever that had already lost an accuracy gate was re-dispatched behind one debug line. Configuration is now checked once, by name, at both start-ups that can reach this KB: the inference_optimizer preflight (after the dotenv loaders, so a value set in .env is caught) and the standalone fa CLI, which cannot rely on that preflight. The resolver is total again, which also makes reader and writer agree under the withdrawn variable instead of one raising while the other wrote on. The advisory except clause stays — it must not block dispatch — but logs at warning, because reaching it means the gate is off. The collision guard compared the framework root against _resolve_local_kb_root, which resolves to <workspace>/knowledge: a path it was never going to collide with. Setting the framework root back to "kb" left the suite green, so the test asserted nothing. The root that matters is the legacy recipe root <workspace>/kb, and the guard now names it; putting the old value back fails it. Every description of "kb" as belonging to "the recipe KB" is narrowed to the legacy root, and the claim that INFERENCE_OPTIMIZER_FA_KB_PATH is the only KB variable the env_safety allowlist passes is dropped: it clears a prefix rule that HYPERLOOM_LOCAL_KB_ROOT clears too. The move to <workspace>/framework-kb had no migration, defended by "no lessons.jsonl was found on the dev box". That argument runs backwards: the writer was working the whole time — that is the premise of the read/write split — so any machine that has run a FRAMEWORK phase has a ledger under <workspace>/kb/framework_optimization. The dev box being empty only says it never ran one, which is the same thing its empty task database says. Orphaning that ledger empties the dedup record and silently re-proposes PRs that already lost an accuracy gate. Start-up now carries the partition across once, following the recipe migration's precedent: only into a destination with no framework data, staged under a sibling directory so an interrupted copy cannot leave a half-populated one, source left in place, skipped when the operator named a root. Co-authored-by: Cursor <cursoragent@cursor.com>
…rants framework_agent declared lease_ttl_sec 3600 while its executor defaults variant_timeout_sec to DEFAULT_VARIANT_TIMEOUT_SEC = 7800: one number written in two places, disagreeing by a factor of two. reclaim_expired_running measures now - updated_at, and updated_at only moves on a state transition — nothing refreshes it while a task runs — so the lease is a total wall-clock budget. A candidate that rebuilds from source and then benches past the hour was marked failed and had its lanes released while its benchmark was still on the GPU, leaving the next task free to restart the server underneath it. Both actions that share that executor default move to 10800: 7800 for the bench the executor already allows, plus room for the clone, the rebuild and the accuracy eval that bracket it. integrate_patch carried the identical contradiction and is fixed with it rather than grandfathered, so the new guard needs no exception list. The margin above 7800 is a judgement call — the surrounding phases are not separately bounded and there is no measured build time to size it from. The guard test asserts only the part that is a contradiction between two declarations: lease_ttl_sec >= DEFAULT_VARIANT_TIMEOUT_SEC. Co-authored-by: Cursor <cursoragent@cursor.com>
_registry_lanes_ttl answers ([], 0) both for an unknown action and for a coordinator whose ActionRegistry failed to load, and that failure is already downgraded to action_registry = None. Sourcing lanes from the registry — right, in itself — routed that path into the one action that git-applies to the live source tree, so a registry that failed to load would enqueue it holding no lanes at all: no server_lifecycle, no workspace_mutation, no benchmark_lane, and TTL 0 back to being watchdog-exempt. The hardcoded lanes it replaced were ugly, but they were a floor. An empty lane set is now a refusal, which the existing handler turns into a warning and an enqueue_failed progress row, so the candidate is skipped and the pump keeps moving. Co-authored-by: Cursor <cursoragent@cursor.com>
…the upgrade The v5 migration renamed the FRAMEWORK fields but left optimization_stack alone, so a session that had already promoted a KEEP kept entries whose variant_name still carried the promote-side "framework:" prefix. Resume reconciliation keys on the bare candidate key, so for the rest of that session those entries read as orphaned KEEPs, and the (action, variant_name) dedup that stops a second append for the same PR stopped matching them too. Not a regression — the pre-branch key missed them the same way — but the branch's test only covered freshly written entries, so the warning looked fixed. Dropping the prefix also changed a quieter case: the old name was truthy even when the candidate key was empty, so a KEEP with no identity was stacked under the name "framework:"; the bare key is falsy, so it is now skipped. Skipping is the better outcome, but a KEEP whose gain never reaches the stack should not disappear without a word, so it is logged. Co-authored-by: Cursor <cursoragent@cursor.com>
… two leftovers INFERENCE_OPTIMIZER_FA_KB_PATH appeared in neither .env.template nor the environment-variables reference. That predates this branch, but it has just gone from a compatibility override to the sole supported entry point, and the new start-up error tells operators to switch to a name they cannot look up. Also: reflow the phase-semantics paragraph, whose line breaks an earlier edit left mid-clause, and stop sending the Critic a rationale key on outcomes that have no rationale — only stamped rows carry one, bench results record their numbers instead. The surrounding try/except went with it: the body walks in-memory dicts and lists it has already type-checked. Co-authored-by: Cursor <cursoragent@cursor.com>
Dropping the blanket except from _collect_framework_agent_candidate_priors exposed that _ReviewCoord never bound _CRITIC_PRIORS_OUTCOME_TAIL: the AttributeError had been swallowed and the priors silently came back empty. The stub borrows the constant like it borrows the methods, rather than the except being restored to hide the gap again. Co-authored-by: Cursor <cursoragent@cursor.com>
CI E2E report — ✅ Succeeded
|
Co-authored-by: Cursor <cursoragent@cursor.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
1 participant
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.
Closes the defects found by the
framework->framework_agentrename audit. Every one of them is a silent failure: the code returns a plausible default instead of erroring, which is why they survived review and why several had no test coverage at all.The recurring shape is the same throughout: one name or path derived independently in two places, then quietly drifting apart. The lane list was written twice, the KB root was resolved twice, resume re-derived a candidate id from a display name, and the executor key was spelled separately from the kind that gets enqueued. So the fixes make one side derive from the other rather than adding compatibility shims.
Behaviour changes reviewers should weigh
framework_agentlease TTLintegrate_patchlease TTLFRAMEWORK_AGENT_KB_DIRINFERENCE_OPTIMIZER_FA_KB_PATH<workspace>/kb(unreadable in practice)<workspace>/framework-kb, with an existing partition carried across once at start-upframework_agentenqueued with no lanes at allenqueue_failedprogress rowstate.jsonschemaOn the TTL value. Every action carries a wall-clock ceiling;
framework_agentwas the only one exempt, and that was the bug. The first version of this branch used the 3600s its metadata already declared, which review showed to be wrong in a way that needed no measurement: the executor defaultsvariant_timeout_sectoDEFAULT_VARIANT_TIMEOUT_SEC = 7800, so one declaration allowed a bench the other would kill at half the distance.reclaim_expired_runningmeasuresnow - updated_at, andupdated_atonly advances on a state transition -- nothing refreshes it during a run -- so the lease is a total wall-clock budget, not an idle timeout. A candidate that rebuilds from source and benches past the hour was therefore markedfailedand had its lanes released while its benchmark was still on the GPU, leaving the next task free to restart the server underneath it.Both actions sharing that executor default now declare 10800s, and a guard test asserts
lease_ttl_sec >= DEFAULT_VARIANT_TIMEOUT_SEC.integrate_patchcarried the identical contradiction and is fixed alongside rather than grandfathered, so the guard needs no exception list.What each commit does
Runtime defects
a0be780cFRAMEWORK enqueue takes its lanes and lease TTL from the action registry instead of hardcoding one and dropping the other.ead3dca7Resume reconciles framework KEEPs on the family label plus the canonical candidate key. The promote path had decorated the stack entry with aframework:prefix nothing consumes, while passing the undecorated key in a field_lift_to_current_bestnever read.386f158d7cabd946857851c5bdbcbb89a2eccf49The KB gets one owner for the mutable ledger shared by reader and writer, a separate packaged root for the read-only seed, its own workspace directory away from the recipe KB's legacy root, and the reader-only override withdrawn.99c8af8eCritic denials are sourced from the progress ledger that actually records them; the field with two readers and no writer is gone. This is not an equivalent swap, and the original wording undersold it: the removed field had zero writers onmain, sorecent_decisionsandlearningswere always empty. The Critic now receives real denial reasons for the first time.c967685cThe per-repo discovery cap becomes the constant it always was, with its PolicyGate lock and robustness mirror removed.e1c856d4state.jsonv5 migrates the FRAMEWORK fields the rename left behind. Ten fields were renamed; eight are migrated and two are the fields removed above.Declarations that did not match reality
819af569The PolicyGate denial hint and the orchestration prompt named an action the runtime does not use.22c40a3a--helpquoted 0.28 and 0.12 for the KERNEL_AGENT and SWEEP budgets against real defaults of 0.35 and 0.05; thephase_breakdownTypedDict declaredkernelwhile the producer emitskernel_agent. The emitted JSON is unchanged.7e9f36c6The framework standalone installer ranpip install -eagainst a directory with nopyproject.toml, so it failed for anyone following the README. Nothing called it and the main installer had already stopped chaining to it.337102547b7af22cd4141a7fComments, the registry-loaded action description, and leftovers of the removed fields.Guards
325f22e8Every Coordinator-internal action must resolve to a registered executor, no registration may carry a name absent from the catalogue, and the two conditions under which a registration may legitimately be missing are pinned so that exception set cannot grow silently. Also pins the framework gain label against both collectors.f1e8da1aTest stub wiring for the enqueue path.Review round 2
3c8d3d5fKB configuration is validated at start-up; the collision guard is aimed at the root that could actually collide; an existing ledger is carried across.d8f7c59fLease TTL raised to cover the bench timeout the executor grants, for both actions that share it.f4112270An empty lane set is a refusal, not a silent downgrade.8f27902ev5 also normalisesoptimization_stack, so in-flight sessions reconcile.c10664f3INFERENCE_OPTIMIZER_FA_KB_PATHdocumented; prompt reflow; emptyrationaleno longer sent.8ab17783The review stub binds the constant the helper reads.New guards elsewhere cover the cross-process KB ledger, the packaging globs, the parser help defaults, the
phase_breakdownbucket names, the schema rename table, the separation of the two KB roots, and the lease/bench-timeout relationship.Verification
Every fix was validated with a negative control that reproduces the defect first, on a remote development host. That discipline caught four of my own guards that were silently scanning nothing -- three during the original work and one that review found, described below.
The strongest single piece of evidence is the cross-process KB check, run side by side against the unpatched tree: with only
USER_DATA_PATHset, the unpatched reader sees zero rows while the patched one reads back what the writer wrote.Suite status: 11663 passed, 7 skipped, 25 failed, 12 subtests passed. Every one of the 25 was replayed against a clean
origin/maintree on the same host: 24 fail there identically, and the 25th,test_setup_cli::test_install_preflights_accept_deepseek_only_without_openai, passes in isolation, so it is the cross-file pollution that predates this branch. No failure is attributable to this branch. The earlier count of 4 pre-existing failures was measured on a different host; this one carries more environment-dependent ones (aiperf shell harness, multi-node bootstrap, robustness monitor, vllm server-patcher).Not verified
No end-to-end run. Everything here is unit-level plus the one cross-process check. No FRAMEWORK phase was executed, no real resume, no benchmark.
The margin above the bench cap is a judgement call. The guard test asserts only the part that is a contradiction between two declarations,
lease_ttl_sec >= 7800. The clone, the source rebuild and the accuracy eval that bracket the bench are not separately bounded, and this host has never run a framework task, so the room between 7800 and 10800 is reasoning rather than measurement. Task durations are recorded in the task history, so the first real run calibrates it for free. Reviewers also suggested a heartbeat viaextend_lease; that machinery does not exist on this path today and there is no data yet to justify building it.Single-machine evidence underlies the claim that nothing sets
FRAMEWORK_AGENT_KB_DIRand that no pre-rename session exists.Corrections to the original description
Review found four claims here that did not hold. Keeping them visible rather than quietly editing them out:
lessons.jsonlneeds migrating" was backwards evidence. The writer was working the whole time -- that is the premise of the read/write split -- so any machine that has run a FRAMEWORK phase does have a ledger at<workspace>/kb/framework_optimization. The dev box being empty only says it never ran one, which is the same thing its empty task database says. The same empty machine was being cited in two directions. A one-time migration is now included._migrate_legacy_recipe_kb_onceis a one-time copy ofrecipe.jsononly; the source is retained and the framework ledger is never touched. The move is still justified --list_domains()reports every directory under its root as a framework domain -- but not by that.INFERENCE_OPTIMIZER_FA_KB_PATHis not "the only KB variable the env_safety allowlist passes." It clears anINFERENCE_OPTIMIZER_prefix rule thatHYPERLOOM_LOCAL_KB_ROOTclears viaHYPERLOOM_too. Corrected in the source comment.framework:prefix has a second effect this description omitted: the old name was truthy even when the candidate key was empty, so such a KEEP was stacked under the name"framework:"; the bare key is falsy, so it is now skipped. Skipping is the better outcome, but it is logged rather than silent.And two defects this branch had introduced, both now fixed:
_resolve_kb_root()put the error on a path that swallows its own failures by design:_config_lever_known_badcaught theRuntimeError, answered "not known bad" for every config, and re-dispatched levers that had already lost an accuracy gate, behind onedebugline. The check now runs once by name at both start-ups that reach this KB.<workspace>/knowledge, a path it was never going to collide with; setting the framework root back tokbleft the suite green. The guard now names the legacy recipe root<workspace>/kb, and putting the old value back fails it.Relationship to the audit note
The note was the starting point, not the specification. Three of its items (#0, #5, #9) were already fixed upstream before this work. #2 is not a defect at the current head -- writer and readers agree -- so it is held by a pinning test rather than the mapping the note proposed. #1 uses a different design from the note's suggested one-line change, because that change would have left reader and writer resolving independently. The grid
HYPERLOOM_FRAMEWORK_*variables are deliberately untouched:HYPERLOOM_FRAMEWORK_PYTHONhas a production reader and the others reach subprocesses outside this repository.The note itself has three errors worth recording: the schema version is 4 rather than 3, the rename commit it cites as
33ac6cccddoes not exist and is really84ad383f, and it lists eight renamed state fields where there are ten.