Repository navigation
refactor(framework): drop the semantic_status mapping nothing produces; correct the KB-ledger docstring and warning; fix three misspellings - #2029
Conversation
|
Gate job conclusion: success (run) on The report below was uploaded by that run; the conclusion above is the result. Code metrics gate: PASSEDJudged: the 7 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Already over at the base (backlog, not this change's): 2
67 findings in files this change does not touchOutside this change (informational): 67
...and 42 more; the job log lists every row.
Thresholds and sources: |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2029 -- refactor(framework): drop the semantic_status mapping nothing produces, plus comment and spelling fixes
What it does: The PR removes the dead framework_audit.semantic_status mapping in FrameworkPhase._record_framework_agent_authoring_empty_outcome, so the recorder stamps author_empty directly and its INFO line drops status=. It also deletes a false KB-ledger comment in integrate_patch.py, fixes three codespell spellings, and in a third commit rewords the kb.py legacy-migration failure WARNING. All of it is in the FRAMEWORK phase and executor layers.
Blocking issues: 1
- [desync-kb-warning-unstated] PR description never mentions the kb.py WARNING text change added by the head commit [verified]
Problem: Head commit 180a05b ("fix(framework-kb): say what a missing ledger costs when the legacy copy fails") changes the operator-visible WARNING inmigrate_legacy_partition_once. It used to end "it may re-propose PRs it has already tried." and now ends "it may re-dispatch config levers that already lost an accuracy gate." body.txt never names kb.py. Its 'Observable effect' section lists only the INFO line in framework.py and the 'unparsable' warning, and its 'Breaking changes' section says anyone matching "the two log lines above" sees new wording, but this diff changes three log lines. The body also says ruff and codespell ran on "the five changed Python files", while files.txt lists six. The title calls the extra changes "comment and spelling fixes", but this one is a rewritten runtime log message. src/hyperloom/agents/framework/kb.py:100
Impact: The release cut builds the GitHub release from PR descriptions. A log message whose wording changes, the same kind of change the body lists for the other two lines, is left out. The 'Commands run at this head' section is also stale for the sixth file.
Action: Add kb.py to the description. Put the WARNING rewording under 'Observable effect' and 'Breaking changes' (three log lines, not two). Change 'five changed Python files' to six, or rerun the checks on all six and say so.
Checked: I read diff.txt, body.txt, commits.txt (via git log --stat), ci.txt (all green), comments.txt (only a code-metrics card, no earlier findings) and openprs.txt. I confirmed semantic_status has no producers at head and traced its history to ec44633 and the values it used to produce. I traced every source of framework_audit and audit through framework.py (:1009, :1201, :1410, :1688, :2013, :2246, :2773-2807). I confirmed that read_pr_ledger and lessons.jsonl have one production reader, _config_lever_known_bad (framework.py:3121), and that kb_writeback only writes. I checked the head numbering of the body's line references (:121, :2627, :2639) and confirmed main has not touched the changed files since the base. I grepped tests, docs and prompts for the changed log strings and found none that would break. I checked the diffs of the overlapping open PRs #1845, #1867, #1997 and #2004 for the recorder, semantic_status and the reworded strings. Rule angles applied: V1-V8, X2/X3 (description against diff at head), core-file tiers (framework.py and integrate_patch.py are backbone files; this change is a dead-branch removal plus comment and log text, covered by the updated tests), and the AI- | Base: b7004d1 | Head: 180a05b
180a05b to
90d082d
Compare
okorzh-amd
changed the title
|
@chaojhou Thanks, you are right on all three counts. The description is now rewritten against 90d082d.
|
90d082d to
a9d3e4b
Compare
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2029 -- refactor(framework): drop the semantic_status mapping nothing produces; correct the KB-ledger comment and warning; fix three misspellings
What it does: removes the semantic_status branch in phases/framework.py, which no producer feeds. It corrects the KB-ledger comment and the agents/framework/kb.py WARNING text; both falsely said the phase would not re-dispatch a lever that lost an accuracy gate. It also fixes three misspellings.
Blocking issues: 2
-
[X2] The description says the code-metrics baseline is unchanged, but the PR lowers it [verified]
Problem: the Size/complexity line says "scripts/code_metrics_baseline.txtis unchanged" and that neither entry can be lowered. At head the PR changesmodule-lines src/hyperloom/orchestrator/phases/framework.pyfrom 3139 to 3132 (git diff bb2da6333 a9d3e4ba6 -- scripts/code_metrics_baseline.txt), and the module sizes quoted next to it are out of date.
Action: state the lowered entry and the current module sizes. -
[X2] The description no longer matches the integrate_patch.py change after the rebase [verified]
Problem: the description says the integrate_patch.py change deletes a comment sentence ending "so a lane that writes none is re-benched forever" at integrate_patch.py:4275-4277. Main no longer contains that sentence. At head the PR rewrites the_maybe_write_framework_kb_recorddocstring instead, and the description does not mention that.
Action: describe the docstring change the PR actually makes.
The earlier blocker ([desync-kb-warning-unstated] at 180a05b) is resolved: the description now states the kb.py WARNING change with the old and new log text. The code itself is correct, and CI is approved and running at this head; only the description needs these two edits.
Checked: the full 7-file diff bb2da63..a9d3e4b (kb.py, _patch_source_pr.py, integrate_patch.py, framework.py, code_metrics_baseline.txt and two test files) | Ran: git diff bb2da6333 a9d3e4ba6 -- scripts/code_metrics_baseline.txt; git merge-tree --write-tree origin/main a9d3e4ba6 (clean) | Head: a9d3e4b
a9d3e4b to
f43169a
Compare
okorzh-amd
changed the title
@chaojhou thanks, both items are addressed in the description, and I rebased the branch onto main
|
…oduces
_record_framework_agent_authoring_empty_outcome mapped
framework_audit.semantic_status onto already_present / not_applicable /
author_empty. framework_audit is the candidate's audit, which discovery
builds as {verdict, reason, recommended_next_step}; no code, prompt or
schema writes semantic_status, so the recorder always stamped
author_empty. Stamp author_empty directly, and drop the fixtures and the
status-variants test that fed the unread field.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…sspellings _maybe_write_framework_kb_record's docstring says a patch with no PR identity leaves no lesson because the ledger has no key "to dedup future discovery against". Discovery does not dedup on the framework KB ledger: its only reader is the config-lever accuracy check (FrameworkPhase._config_lever_known_bad), which matches on the config fingerprint, not on the PR. Say what actually stops the write: the ledger files every lesson under a PR URL or head SHA. codespell flags "applyable" twice in _patch_source_pr.py and "unparseable" in a FRAMEWORK config-lever warning. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…py fails The FRAMEWORK phase reads the PR ledger only to skip a config lever whose fingerprint already lost an accuracy gate; nothing dedups proposals on it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
f43169a to
0600ee5
Compare
Resolved at the rebased head; superseded by the re-review.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #2029 -- refactor(framework): drop the semantic_status mapping nothing produces; correct the KB-ledger docstring and warning; fix three misspellings
What it does: it removes the semantic_status mapping in FrameworkPhase._record_framework_agent_authoring_empty_outcome, which no producer feeds, so the recorder stamps author_empty directly. It also:
- corrects the KB-ledger docstring of
EnablementGateMixin._maybe_write_framework_kb_record(_integrate_enablement_gate.py:597-598); - corrects the
agents/framework/kb.pylegacy-copy WARNING; - fixes three misspellings;
- lowers the framework.py module-lines baseline from 3051 to 3038.
Behaviour is unchanged apart from the stated log lines.
Blocking issues: none
Both blockers from the previous review (at a9d3e4b) are resolved at f43169a:
- Baseline. The description states the lowered framework.py entry and the current sizes, and the baseline diff is exactly that one lowered line.
- integrate_patch.py. The description now describes the docstring rewrite the PR actually makes, in
_integrate_enablement_gate.py, and the PR no longer touches integrate_patch.py.
The branch merges cleanly with current main 7f1c5e8.
The 02:24 push only rebased onto 267a0f4; git range-diff shows the three commits unchanged from f43169a apart from one context line in code_metrics_baseline.txt (main's own prelude.py entry). It merges cleanly with current main, and CI was approved at this head.
Checked:
- the full 7-file diff 8b0ae55..f43169a;
- every description line reference at head;
- callers of the changed recorder and
_record_undelivered_authoring_attempt; - that
semantic_statusappears nowhere at head, and thatread_pr_ledger's only reader matches the corrected wording.
Ran:
git merge-tree --write-tree origin/main f43169a10(clean);- the author's named test files plus test_kb.py and the integrate_patch helper suites on head and on main 8b0ae55. The failing ids are identical on both and are Windows-only.
Head: 0600ee5
LGTM
Description: what and why
Everything below describes head
f43169a10, rebased ontomainat8b0ae5500(origin/main at the time of the rebase). Since the previous head, main's refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098 splitintegrate_patch.pyinto one module per integration stage, and the KB-ledger docstring this PR rewrites moved with it intoactions/executors/_integrate_enablement_gate.py. Main's refactor(framework-phase): read a model-authored payload through one coercion module #1997 moved the config-lever warning into_config_lever_from_proposal, where its wording is now afaultstring. The PR follows both moves and re-inlines nothing.FrameworkPhase._record_framework_agent_authoring_empty_outcome(phases/framework.py:2487) stamps the progress row of a FRAMEWORK candidate whose authoring specialist delivered nothing. Before stamping, it mappedparams["framework_audit"]["semantic_status"]:already_equivalent/already_supersetbecamealready_present;not_present/partially_presentbecamenot_applicable;author_empty.Nothing writes
semantic_status.framework_auditis the candidate'saudit, which discovery builds as{verdict, reason, recommended_next_step}(phases/framework.py:2719-2723). The review pump, the Critic proposal, the Critic-approved materialization and the authoring dispatch carry it unchanged (:1039,:1939,:1992,:1219). The two re-author paths copy it from the Critic proposal or the earlier specialist task (:1362,:2203,:2217), and the local-explore dispatch passes{}(:1676). Since refactor(framework-phase): read a model-authored payload through one coercion module #1997 most of these hops go throughto_mapping, which copies a mapping and turns anything else into{}; it adds no key. No file in the repository names the field outside the code and tests this PR removes. Discovery also drops proposals whoseverdictisalready_presentornot_applicablebefore they become candidates (:2689-2691), so an authoring candidate never carries either. The mapping always fell through, and the recorder always stampedauthor_empty.What changes (six Python files and the code-metrics baseline):
phases/framework.py). The recorder stampsauthor_emptydirectly (:2540), and its INFO line drops thestatus=field that could only readauthor_empty(:2553). Since [fix] close the defects behind the MuseSpark run shortfall and delete state nothing reads #2055, the recorder also passes the stamped status to_record_undelivered_authoring_attempt, which charges the empty round against the candidate's lever in the attempt ledger. That call now passes the same"author_empty"literal (:2552). The helper and its signature stay as main has them, because the dispatch-failure recorder also calls it withdispatch_failed(:2480).actions/executors/_integrate_enablement_gate.py). This is a rewrite of the docstring ofEnablementGateMixin._maybe_write_framework_kb_record(_integrate_enablement_gate.py:587); the PR no longer touchesintegrate_patch.py. The docstring said a patch with no upstream-PR identity "is a one-off edit the ledger has no key to dedup future discovery against". This PR first targeted a comment in the same method that made the same claim ("so a lane that writes none is re-benched forever"). Main's refactor(integrate-patch): resolve, apply and grade one patch through named stages #1998 removed that comment and put the claim into the docstring, and refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098 then moved the method, docstring included, out ofintegrate_patch.pyinto this module, so the fix followed it. The ledger's only production reader isFrameworkPhase._config_lever_known_bad(phases/framework.py:3008, reading at:3017/:3023). It matches rows on a config fingerprint (applicability) and a negative accuracy delta, never on the PR. The docstring now gives the real reason a patch with no identity leaves no lesson: "the ledger files every lesson under a PR URL or head SHA" (_integrate_enablement_gate.py:597-598). The method returns early with a warning when the proposal has neither (:612-614).agents/framework/kb.py).migrate_legacy_partition_oncelogs a WARNING when the legacy KB partition cannot be copied (kb.py:97-104). Its last sentence said a missing ledger means the phase "may re-propose PRs it has already tried". It now says what a missing ledger actually costs: the phase "may re-dispatch config levers that already lost an accuracy gate" (kb.py:100). This is a change to operator-visible log text, not only a docstring; see Observable effect.actions/executors/_patch_source_pr.py,phases/framework.py). codespell flags three misspellings in these files:_patch_source_pr.pycomments (:207,:295);faultstring of the FRAMEWORK config-lever warning in_config_lever_from_proposal(phases/framework.py:65), which is also log text.scripts/code_metrics_baseline.txt). The entrymodule-lines src/hyperloom/orchestrator/phases/framework.pyis lowered from 3051 to 3038, the file's new length. No entry is raised or added.The other two changed files are the tests below.
Linked issue(s): none.
Tests: added/updated? commands run? (what to test; if a test was replaced, where its assertions live now): updated, no tests added. Both in
src/hyperloom/inference_optimizer/tests/:test_framework_agent_authoring.py: five tests drop theframework_audit: {"semantic_status": "not_present"}they passed (:648,:767,:803,:842,:879). Three of them expectednot_applicable, only because of that field, and now expectauthor_empty(:674,:875,:918).test_framework_agent_selection_audit.py:test_record_authoring_empty_already_presentbecomestest_record_authoring_empty_stamps_author_empty_once(:544). Its fixture dropssemantic_status: already_equivalent, it expectsauthor_empty, and it keeps its idempotency assertion (a second call appends no duplicate row).test_record_authoring_empty_status_variantsis deleted. Its "no audit givesauthor_empty" assertion lives on in:544and the authoring tests above. Itsnot_present→not_applicableassertion goes with the branch it covered.Main's
test_framework_dispatch_failure_not_authored_empty.pyalready covers the attempt-ledger row for an empty round (test_an_undelivered_round_counts_once_against_the_source_lever,:101, whose empty-deliverable case expects outcomeauthor_empty, andtest_an_empty_upstream_authoring_round_counts_against_the_upstream_lever,:118). It passes unchanged.No test asserts the changed wording of the three log lines:
src/hyperloom/agents/framework/tests/test_kb.py:218matches only the unchanged opening of the KB warning ("could not carry the legacy partition over"), and a search of tests, docs and prompts finds none of the old strings.Commands run at this head (
f43169a10, on main8b0ae5500):pytestontest_framework_agent_authoring.py,test_framework_agent_selection_audit.pyandtest_patch_source_pr.py(insrc/hyperloom/inference_optimizer/tests/) andtest_integrate_patch_helpers_unit.pyandtest_integrate_patch_executor.py(insrc/hyperloom/orchestrator/actions/executors/tests/): 284 passed. With every other test file that references the changed functions or the KB ledger readerread_pr_ledger(src/hyperloom/agents/framework/tests/test_kb.py;test_framework_dispatch_failure_not_authored_empty.py,test_framework_kb_models_units.pyandtest_framework_switch_manifest.pyinsrc/hyperloom/inference_optimizer/tests/;src/hyperloom/orchestrator/actions/executors/tests/test_integrate_patch_coverage_unit.py): 454 passed.test_framework*.pyinsrc/hyperloom/inference_optimizer/tests/, everytest_integrate*.pyinsrc/hyperloom/orchestrator/actions/executors/tests/,test_kb.py,test_patch_source_pr.pyandtest_explore_executor.py(31 files) give 1052 passed and 2 skipped at every commit.-n 32,-k 'not kernel_entry_auto_runs'): 26035 passed, 45 failed, 26 errors and 31 skipped at head. Every failing or erroring id, re-run on main8b0ae5500in the same environment, fails or errors there too (45 failed, 26 errors), so the PR adds none. Most causes are host-specific: missing TraceLens, a ROCm install the profiler tests do not expect, ownership checks on this host's temporary directories (git'ssafe.directoryand the multi-node state file), git's default branch, and a GPU held by another process, which makes the GEAK handoff refuse withgpus_busy_at_handoff. One is a main breakage outside this PR:test_coordinator_async_batch2_unit.py::test_a_candidate_after_resume_still_sees_the_kept_source_layerstill callsintegrate_patch._git_head_sha, which refactor(integrate-patch): give each stage of an integration attempt a module of its own #2098 stopped importing intointegrate_patch.py.ruff checkandruff format --checkon the six changed Python files: pass (6 files already formatted).lint-imports: 10 contracts kept, 0 broken.python scripts/code_metrics.py --base-ref origin/main: PASSED (0 new, 0 worse, 0 baseline entries out of date in every dimension).scripts/check_cli_references.py, andscripts/check_broad_suppress.pyon the six changed Python files: pass.scripts/pr_hygiene.py size --base origin/main --head HEAD: 31 production Python lines (size/M).Size/complexity triggers crossed: split, or why the shape is right: none crossed. The diff is 7 files, +16/-63, and the code-metrics gate reports 0 new and 0 worse units. The recorder goes from 80 to 67 lines (main had it at the 80-line limit), from cyclomatic complexity 12 to 10, and from cognitive complexity 24 to 20.
phases/framework.pyshrinks from 3051 to 3038 lines. On main it sits at its 3051-line baseline entry and over the 1200-line limit. It is still over the limit, so this PR lowers the entrymodule-lines src/hyperloom/orchestrator/phases/framework.pyfrom 3051 to 3038; that is the only change toscripts/code_metrics_baseline.txt. The docstring fix replaces two lines with two, so_integrate_enablement_gate.py(657 lines) and_maybe_write_framework_kb_recordkeep their length.If this simplifies or refactors: which mechanism was removed, which contract was preserved, and the tests that show it: removed the
semantic_status→ status mapping, a decision path that read a field no producer writes. Preserved: an empty authoring deliverable stamps exactly oneauthor_emptyrow per candidate, not kept, and only when the deliverable carries no patches, config levers or routable artifacts. The attempt-ledger row for that round keeps the outcomeauthor_empty. Tests:test_framework_agent_selection_audit.py:525and:544,test_framework_agent_authoring.py:648,:767,:803,:842,:879, andtest_framework_dispatch_failure_not_authored_empty.py:101and:118.Observable effect: what an operator will now see, and what the old behaviour cost them (if none, why the change is unobservable):
author_emptybefore, because the mapping never matched.phases/framework.py):FRAMEWORK: authoring specialist empty deliverable candidate=... batch=...no longer ends withstatus=author_empty.phases/framework.py): the FRAMEWORK config-lever warning reads "has unparsable server args" instead of "has unparseable server args".agents/framework/kb.py): the FRAMEWORK KB warning logged when the legacy partition cannot be copied now ends "so it may re-dispatch config levers that already lost an accuracy gate." instead of "so it may re-propose PRs it has already tried."The old code cost a reader and an operator a false lead. The branch suggested the audit could stamp
already_presentornot_applicable. Both the KB docstring and the KB warning pointed at a PR dedup that does not exist. An operator who saw the warning would look for repeated PRs, not for a config lever re-run that the accuracy-gate ledger would have skipped.Breaking changes: no. No field, status value, artifact or flag changes. Anyone matching the exact text of the three log lines above (the empty-deliverable INFO line, the config-lever "unparsable" WARNING and the KB legacy-copy WARNING) sees the new wording.
PR addresses single concern: no. The main change removes the dead
semantic_statusmapping. Two small, independent text fixes ride along, each too small for a PR of its own and neither changing control flow: the false KB-ledger claim (in one docstring and in one runtime WARNING), and the three misspellings codespell reports in these files (one of them in a WARNING).Root cause is upstream (Magpie/TraceLens/GEAK/IntelliKit/AgentKernelArena), ticket filed: no.