Repository navigation
refactor(kernelforge): delete the re-checks and re-reads in the Analysis service's checkpoint, published-evidence and bundle paths - #2080
Conversation
…idator apply_published_evidence re-checked the manifest commit and digests before handing the same files to _validate_bundle, which already rejects every one of those mismatches first, before it reads any case or writes provenance. The pre-checks are gone, and the result is built by AnalysisBundle.apply, which already sets canonical_commit, evidence_commit, evidence_stale and the speedup and cumulative-diff fields the trailing replace() re-assigned to the same values. Only the stale branch rebuilds anything now. The canonical-commit override on the validation context is dropped: _validate_bundle reads only its analysis_commit and cases. The case-tuple construction repeated in ensure_bundle, _run_analysis_session and apply_published_evidence becomes _analysis_cases(). evidence_commit is no longer stripped or coerced: every writer of run_state.analysis.evidence_commit stores a bare commit hash. Co-authored-by: Cursor <cursoragent@cursor.com>
apply_checkpoint checked that workflow.json and artifact_catalog.json were regular files before reading them, then read request.json under a second, identical handler. A missing or unreadable file already fails the read with OSError, so the three reads share one handler and the shape checks become one condition. TypeError and JSONDecodeError left the handlers: nothing in them raises the first, and the second is a ValueError. The per-case flagging moves to _checkpoint_case_evidence, beside the _completed_ and _skipped_case_evidence helpers it mirrors. Its bottleneck read suppresses the same errors and keeps the case's own bottleneck on failure, as the reassignment in the old handler did. Co-authored-by: Cursor <cursoragent@cursor.com>
…ated After publishing a generation, ensure_bundle read manifest.json, workflow.json and request.json back from disk although every value it took from them was already in hand: the manifest status is in the bundle _validate_bundle returned for the staging root, the attempt count is in the journal that just wrote workflow.json, and the request's profiling flag adds nothing to completed_case_ids, which validation already rejects for a static-only request. The final AnalysisBundle rebuild becomes replace(validated, outcome=...). The final_root.is_dir() guard before _published_generation_root goes: the resolver returns None for a missing or non-directory commit root. _bundle_from_published_checkpoint derives requested_tier from profiling_enabled instead of taking it as a parameter, _protected_paths takes the AnalysisSources it used to receive unpacked, the kernel path is resolved once, and the CancelledError and AnalysisAttemptLimitError handlers, which did the same thing, share one clause. The rebinding of request_payload to the durable request was never read. What is left moves into three steps named for their job: _stage_incremental_diff, _stage_request and _publish_session_bundle. The order of every write, git call and raise is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Staging the incremental diff and writing the request are separate steps now, and each built the diff path from its own copy of the file name, as _session_outputs did. _INCREMENTAL_DIFF_FILE is the one spelling the three share. Co-authored-by: Cursor <cursoragent@cursor.com>
apply_checkpoint, apply_published_evidence and ensure_bundle are within the cognitive and function-length limits, so their five entries go. _run_analysis_session is 94 lines and the module 1953. Co-authored-by: Cursor <cursoragent@cursor.com>
Code metrics gate: PASSEDJudged: the 3 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
90 findings in files this change does not touchOutside this change (informational): 90
...and 65 more; the job log lists every row.
Thresholds and sources: |
fengshaoyi-amd
changed the title
ZhengGong-amd
left a comment
ZhengGong-amd
left a comment
There was a problem hiding this comment.
Please reconsider one point before merge.
- The new
_checkpoint_case_evidence(src/kernelforge/orchestrator/analysis.py:1830-1832) carries its own read ofbottleneck.json(with a newcontextlib.suppressandimport contextlib), while the owner_case_bottleneck(:1856) sits next to it. The two already diverge: a JSON array inbottleneck.jsonmakes this copy raiseAttributeErrorand fails the wholeapply_checkpoint, while_case_bottleneckreturns{}. The inline copy predates this PR, but a PR whose goal is deleting repeated code should not wrap it into a new function. AGENTS.md Clean design.
Action:bottleneck = str(self._case_bottleneck(bottleneck_path).get("classification") or original.bottleneck), dropcontextlib, and note in the description that a non-objectbottleneck.jsonno longer failsapply_checkpoint.
Co-authored-by: Cursor <cursoragent@cursor.com>
…orch-analysis-pr Co-authored-by: Cursor <cursoragent@cursor.com>
…orch-analysis-pr Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review.
|
xiaofei-zheng
left a comment
xiaofei-zheng
left a comment
There was a problem hiding this comment.
PR #2080 -- refactor(kernelforge): delete the re-checks and re-reads in the Analysis service's checkpoint, published-evidence and bundle paths
What it does: Three AnalysisAgentService methods in src/kernelforge/orchestrator/analysis.py -- apply_checkpoint, apply_published_evidence and ensure_bundle -- were over the code-metrics cognitive-complexity and function-length gates because they re-ran checks _validate_bundle already makes and re-read manifest.json, workflow.json and request.json for values that were already in hand. The PR deletes those duplicates, extracts five single-call-site helpers (_analysis_cases, _checkpoint_case_evidence, _stage_incremental_diff, _stage_request, _publish_session_bundle), lets the validated bundle and the session journal supply the manifest status, attempt count and digests, and lowers the two remaining baseline entries. A follow-up commit routes the checkpoint path through the shared _case_bottleneck so a non-dict or non-UTF-8 bottleneck.json no longer escapes as an uncaught exception.
Blocking issues: 1
- [X2] The PR description is pinned to commit
f0fa2d510, not to head53d1339bb[verified]
Problem: the Checklist states "No test is added, changed or deleted", but the head diff changessrc/kernelforge/tests/test_analysis_agent.py(+13/-0): the_SourceThenFailBackendfixture now writesanalysis.mdandbottleneck.json, andtest_partial_checkpoint_is_exposed_as_orchestration_evidencegained three assertions. Three stated counts belong to the earlier commit as well -- "Production code: +222 / -349 (net -127)" and "2,080 -> 1,953" (head is222 350, net -128, andanalysis.pyis 1952 lines; it was 1953 atf0fa2d510and 1952 after20ec02642), the Gate section reports a pass "on the 2 touched files" when three files are touched, and the commit table lists five commits while the branch carries six.scripts/code_metrics_baseline.txt:762already carries the correct 1952.
Impact: a reviewer checking the "contract preserved, no test changed" claim against the diff finds a test change the description denies, and the release cut aggregates this description.
Action: refresh the Checklist's Tests line, the production-line counts, the module-length figures, the touched-file count and the commit table against head53d1339bb.
Checked: src/kernelforge/orchestrator/analysis.py (ensure_bundle, _stage_incremental_diff, _stage_request, _publish_session_bundle, _bundle_from_published_checkpoint, apply_checkpoint, apply_published_evidence, _checkpoint_case_evidence, _case_bottleneck, _completed_case_evidence, _validate_bundle, _validated_manifest, _validated_session_metadata, _published_generation_root, _tier_label, AnalysisBundle.apply), src/kernelforge/orchestrator/analysis_session.py (AnalysisSessionJournal.attempts, finalize), src/kernelforge/loop/analysis_runtime.py:80-100, scripts/code_metrics_baseline.txt, src/kernelforge/tests/test_analysis_agent.py | Ran: direct call of _checkpoint_case_evidence with bottleneck.json = "[]" and with a valid object; per-commit wc -l of analysis.py across bb2da63..53d1339; pathlib glob-over-missing-directory check; pytest -k partial_checkpoint_is_exposed (fails on this Windows host inside durable_io.fsync_directory, the platform limitation the body documents -- Linux CI is green at head) | Base: bb2da63 | Head: 53d1339
SKIPPED: Step 7 independent reader -- no second agent or person available; the finding was re-derived from the artifacts alone instead.
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
Verified at 53d1339: the checkpoint path reads bottleneck.json through _case_bottleneck and contextlib is gone. Every review item on the code is fixed, CI is green and the branch merges cleanly with main.
Before or right after merge, please refresh the PR description against this head (line and module counts, commit table, and the statements xiaofei-zheng / ZhengGong-amd listed): the release notes are built from it.
LGTM
What this does
Three methods of
AnalysisAgentServiceinsrc/kernelforge/orchestrator/analysis.pywere over the code-metrics gate:apply_checkpoint,apply_published_evidenceandensure_bundle. This PR deletes what they re-checked or re-read: checks_validate_bundlealready makes, second reads ofmanifest.json,workflow.jsonandrequest.jsonfor values already in hand, guards on inputs no caller produces, and a case-tuple block written three times. The checkpoint path now readsbottleneck.jsonthrough the shared_case_bottleneck, which also accepts a file that is not UTF-8.Production code: +222 / -350 lines (net -128) in 1 file. Tests: +13 / -0 in 1 file. Code-metrics baseline: 5 entries retired, 2 lowered, none added.
What changes
apply_published_evidence: the manifest read and the commit and digest checks_validate_bundlemakes first are deleted, with a canonical-commit override_validate_bundlenever reads and a trailingreplace()of fieldsAnalysisBundle.applyalready sets.apply_checkpoint:is_file()checks before reads that fail withOSErroranyway and a second identical handler aroundrequest.jsonare deleted;_checkpoint_case_evidencebuilds a case's evidence from the checkpoint and reads itsbottleneck.jsonthrough_case_bottleneck.ensure_bundle: the re-reads ofmanifest.json,workflow.jsonandrequest.jsonafter publishing are deleted, since the values come from the validated bundle and the journal that wrote them, with duplicateCancelledErrorandAnalysisAttemptLimitErrorhandlers and a hand-rebuiltAnalysisBundle._stage_request,_stage_incremental_diffand_publish_session_bundleare its staging and publishing steps._analysis_casesbuilds the case tuple once, replacing three copies.Functions brought under the gate
Baseline entries this PR retires, with their values at base:
analysis.py:AnalysisAgentService.apply_checkpoint(cognitive 39, lines 138);AnalysisAgentService.ensure_bundle(cognitive 39, lines 219);AnalysisAgentService.apply_published_evidence(lines 96)Baseline entries this PR lowers:
AnalysisAgentService._run_analysis_sessionlines: 101 → 94analysis.pymodule lines: 2,080 → 1,952Observable effect
A malformed
bottleneck.jsonin an Analysis case no longer fails the Analysis service:AttributeErrorout ofapply_checkpoint; the case now keeps the bottleneck it had.UnicodeDecodeError; it now reads as an empty bottleneck, as an unreadable or invalid one already did.Bundles, generations, request, workflow and catalog files, recorded outcomes and log lines are otherwise the same for every input the repo's own callers produce. Inputs that no in-repo caller produces now behave differently:
apply_published_evidencewith a padded or non-stringevidence_commit. A padded value no longer matches a bundle, so the context comes back unchanged; a non-string value raisesTypeError. The only production caller passes the bare commit hash stored inrun_state.analysis.evidence_commit.apply_published_evidencewhensource_map.mddisappears after validation. The stale source-map reference is included instead of left out.apply_checkpointwhenworkflow.jsonorartifact_catalog.jsonis a FIFO or a device. It reads the file, and a FIFO blocks; the framework writes both by atomic replace.apply_checkpointwith a catalog path containing a NUL byte. It raisesValueErrorinstead of skipping the entry.ensure_bundlewhen a bundle file changes between validation and publication. The published status, attempt count and tier come from the values validation checked and the journal that wrote them, not from a re-read.Checklist
test_analysis_agent.py, the_SourceThenFailBackendfixture also writes a case'sanalysis.mdand abottleneck.jsonholding a JSON array, andtest_partial_checkpoint_is_exposed_as_orchestration_evidencenow asserts that such a checkpoint is still exposed, with the case's original bottleneck, its partial analysis and the interpretation-only flag. No other test changed. Linux CI is green at head53d1339bb._run_analysis_session: it was over the 80-line limit at base, changes only where its copy of the case tuple became_analysis_cases, and its baseline entry is lowered from 101 to 94 lines (cyclomatic 11, as at base). Over the 60-line review trigger and within the gate, each one ordered sequence:ensure_bundle(79 lines) andapply_checkpoint(77).analysis.pystays over the 1,200-line module limit it was already over; its baseline entry is lowered from 2,080 to 1,952 lines.bottleneck.jsonno longer fails the Analysis service; nothing else changes for in-repo callers. See Observable effect._bundle_from_published_checkpointdrops itsrequested_tierparameter and_protected_pathstakes theAnalysisSourcesinstead of separate paths; both are private.bottleneck.jsonchange is in the shared read those methods now use.Commits
Squash-merged as
421191de3from head53d1339bb.5ae575fbcrefactor(analysis): restore published evidence through the bundle validator4f9aa18bcrefactor(analysis): read checkpoint metadata in one passc842a5f76refactor(analysis): stop ensure_bundle re-reading the bundle it validatedf0fa2d510refactor(analysis): name the staged incremental diff file once124a9e1adrefactor(analysis): lower the analysis.py code-metrics baseline20ec02642fix(kernelforge): share checkpoint bottleneck parsingorigin/main