Overhaul SDK evolution workflow#40
Conversation
ebarti
left a comment
There was a problem hiding this comment.
Review — strong structure, not ready to merge
The plan's architecture landed well: preflight is complete (including refusing --draft-pr on the default branch), the apply→verify→restore transaction shape is right, subprocess failures become evidence instead of crashes, the resolver is real lockfile-diffing that never mutates the workspace, stage schemas go through the kit's typed bridge with the reviewer enum enforced by Literal, adapter contracts are single-sourced, and the docs-conformance test closes the runbook-drift class. All gates are green (ruff, mypy incl. the example, 280 tests, 89.56% coverage).
However there is 1 critical bug (reproduced by execution) that kills the pipeline's primary happy path, 3 major bugs in the new safety machinery, and the layer containing the critical bug has zero test coverage — nothing in the suite references promote_baseline, load_baseline, or observed_version, and the E2E test's monkeypatched snapshots omit observed_version entirely. See the inline comments for each finding.
Merge blockers
promote_baselinerefuses promotion after every successfully applied update (inline:current_state.py), and the refusal path demotes without rolling back (inline:cli.py).- Cap raise rewrites only the first of two specifier occurrences, so it never takes effect (inline:
resolver.py). - Rollback deletion is scope-inverted: out-of-scope new files survive rollback (inline:
cli.py). - AI-implementation edits are never staged into the PR commit (inline:
cli.py). - Missing tests for the baseline/provenance layer (plan T4.1/T4.2). The one E2E to insist on: update applied → promoted → PR created, with
observed_versionpopulated — that is the first scenario production will hit and the current suite provably doesn't cover it.
Minor (non-blocking)
- Silent skip when
raise_package_cap_in_workspacereturnsNoneafter gates passed (cli.py:474). resolve_constraint_horizon_candidatesreturns()on resolver failure without recording the CommandResult.- The plan's stdout-regex canary test (T2.1) wasn't added — the regex is gone today, the canary keeps it gone.
gh pr create --body <full report>can hit ARG_MAX with a large report — prefer--body-file.- Branch-name timestamp (
parse_args) and report run-id (run_agent) are computed separately and can differ. - Supersede check is silently skipped when
gh pr listfails.
Suggested acceptance before un-drafting
Fix the blockers with their tests, then run the plan's Stage D rehearsal in a scratch clone: forced verification failure (workspace restored byte-identical), forced out-of-scope edit (rolled back AND deleted), and a permissive-fixture --mode upgrade reaching applied && promoted && PR step.
ebarti
left a comment
There was a problem hiding this comment.
Re-review of 4174575 — all merge blockers resolved
I re-verified every finding from my previous review against 41745756, re-ran the reproductions that originally demonstrated the two executable bugs, and re-ran the full gates locally (ruff clean, mypy clean over 34 files, 288 passed / 3 skipped, kit coverage 89.56%). CI is green on this head.
Blockers — all fixed, all with tests
promote_baselinerefusal loop — fixed and fail-closed. Mismatch is now internal provenance (observed_versionvsrequested_version), and promotion selects the snapshot whoseobserved_versionmatches the post-update lockfile, refusing when none does. I re-ran the original reproduction plus three adversarial variants: healthy post-update promotes (snapshot written with correctobserved_version); probe-imported-wrong-version, stale-only snapshots, and legacy no-observed_versionsnapshots all refuse with precise reasons. Covered bytest_promote_baseline_selects_snapshot_matching_updated_lockandtest_promote_baseline_refuses_snapshot_provenance_mismatch.- Demotion without rollback — fixed with the right semantics. Refusal no longer lies about
applied; it recordspromotion_blocked_reason, the PR step is now gated onpromoted, andpr_skipped_reasondistinguishes promotion failure from apply failure. - Cap raise single-occurrence — fixed.
pattern.subnow rewrites every occurrence; my reproduction shows both the extra and thealllist raised to<0.4, the other package untouched, andexclude-newerpreserved — the fix also removes the latent unconditionalexclude-newerstripping from real workspace mutations (now opt-in, and no caller opts in). Bonus defense:_cap_raise_lock_mismatchesverifies post-uv lockthat requested versions actually landed, rolling back otherwise (test_cap_raise_postcondition_detects_unapplied_lock_version). - Rollback scope-inverted — fixed. Deletion now applies to any non-protected stray (verified by
test_restore_transaction_deletes_out_of_scope_strays_with_injected_status, which injects git status and asserts thesrc/stray is deleted while snapshots are restored).command_runneris threaded through, fixing the hardcodedNoneas well. - AI edits never staged — fixed.
changed_pathsis recorded in the implementation result and staged (deduped) into the PR commit;test_create_autonomous_pr_stages_dynamic_in_scope_changes_and_body_filecovers it. The insisted-on E2E now exists:test_run_agent_autonomous_pr_pathdrives a fakeuv lockthat rewrites the lockfile, then asserts applied → promoted → PR created, baselinestatus == "current", and the promoted snapshot'sobserved_version == "0.3.0".
The medium inline findings are also fixed: .sdk-evolution/ removed from AI-editable prefixes (now snapshot-restored and delete-protected on rollback instead), with_kit_version_impact forces manual_design_required unless there is an executable plan, blocked_by_cap is only set when the candidate actually violates the original cap so cutoff_delayed_until is reachable (test_horizon_candidates_distinguish_cap_blocked_from_cutoff_delayed, plus a PEP 440 ordering test for _version_key), and the workflow uses checkout@v7 / setup-python@v6. Of the minor items: --body-file, fail-loud supersede listing, unified run_id, and the recorded cap-raise no-op are all done.
Remaining — none blocking
resolve_constraint_horizon_candidatesstill returns a bare()on temp-resolve failure (resolver.py:75) without recording theCommandResult— report-side observability only.- Observation: after a rolled-back out-of-scope AI edit, a new file under
.sdk-evolution/survives rollback (delete-protected). Since.sdk-evolution/is tracked, preflight's dirty-workspace check blocks the next implementation run until it's cleaned — fail-closed, but worth knowing the failure mode is "manual cleanup", not "auto-heal". - Nit:
promote_baselinewould promote with zero snapshot hashes if every snapshot carriedimport_error(unreachable today — behavior gates block import errors upstream). A one-line refusal whenselected_snapshotsis empty while versions exist would make it locally airtight.
Verdict
This now satisfies the review's acceptance bar: every blocker fixed with its test, reproductions confirmed by execution. Last step before un-drafting, per the original acceptance note: the Stage D rehearsal in a scratch clone (forced verification failure → byte-identical restore; forced out-of-scope edit → rollback + deletion; permissive-fixture upgrade reaching applied && promoted && PR). The new unit tests cover these seams with injected runners; the rehearsal confirms them against real git/uv.
ebarti
left a comment
There was a problem hiding this comment.
Independent review at head 476dc0c (the two prior review rounds were at e3bd222 / 4174575; head adds one further commit that removes the scheduled-report workflow).
Verified fixed at head, by inspection + execution: all 4 prior merge blockers (promote_baseline provenance selection, promotion-refusal keeps applied truthful / PR gated on promoted, cap-raise rewrites every specifier occurrence with a post-lock postcondition, rollback deletes out-of-scope strays via the threaded runner, AI changed_paths staged into the PR) and the prior medium/minor items. Gates green in a clean worktree: ruff clean, mypy clean over 34 files (example now type-checked), 71 SDK-evolution tests pass; T1.4 (check=True) and T2.1 (stdout Update\s regex) grep gates are clean; T3.4 single-sources the adapter contract (zero max_budget_usd in examples/).
One inline Medium below (self-adaptation/kit-staleness path unreachable without an SDK bump).
Low / non-blocking (folded here — root cause is not a diff line): the final commit "Remove SDK evolution report workflow" deletes the T4.3 scheduled workflow but keeps examples/sdk_evolution_agent/status_issue.py (247 lines), which now has no production caller — only python -m ...status_issue and its tests reference it. T4.3's exit criteria (scheduled workflow) is therefore undeliverable and the PR description still advertises "scheduled status reporting, rolling issue support," which no longer matches head. Either restore the workflow or drop the description claim / mark status_issue.py as a manually-invoked utility. Not a correctness bug (the module still works when invoked directly and is tested), and the runbooks/docs contain no stale references to the removed workflow.
Gate: PASS (Blocker=0, High=0).
| f"Raised {package} upper bound {raised.current} -> {raised.replacement}" | ||
| ) | ||
|
|
||
| if not packages: |
There was a problem hiding this comment.
PR Reviewer - review-worker
Severity: Medium
Gate: Pass
Evidence: _run_local_sdk_update returns {applied: False, blocked_reason: "no resolver-selected SDK updates"} here whenever packages (adoptable update_versions plus any gated cap-raises) is empty — this runs before the AI implementation stage at cli.py:553 (if _needs_ai_implementation(architecture)). But evaluate_implementation_gate (stages.py:347) can still return allowed=True with no SDK bump: with_kit_version_impact/with_recursive_impact only force manual_design_required when there is no executable examples/sdk_evolution_agent/ plan, so a kit-version-staleness run (T4.5) or a docs/test-only self-adaptation with an executable plan reaches this function with allowed=True and empty packages.
Reproduction: Reasoned from code. The fake/permissive fixture (stages.py _fixture_payload) always emits self_adaptation_plan: [], and the fixture E2E always has an SDK bump, so no test drives implementation-applied with zero SDK candidates (confirmed by grep over tests/).
When there is no vendor lock bump, the AI implementation / self-adaptation stage is unreachable: the architecture plan passes the gate but is silently discarded and the run reports applied: False with the misleading reason "no resolver-selected SDK updates". This defeats T4.5 (the example's staleness against the kit should trip the recursive gate and then self-adapt) and the docs/test branch of T3.3. It is fail-closed (no workspace damage), but the closed-loop self-adaptation behavior the plan requires never executes.
Required fix: Do not early-return on empty packages when _needs_ai_implementation(architecture) is true — skip only the run_lock_update step and still run the write-enabled implementation stage, scope check, verification, and post-review so an executable self-adaptation/docs-test plan is applied and promoted. Add a test that runs run_agent with implementation enabled, zero SDK update candidates, and an executable self_adaptation_plan naming an examples/sdk_evolution_agent/ file, asserting the implementation stage runs.
Summary
Overhauls the SDK evolution example into a guarded, transactional workflow:
.sdk-evolution/baseline mechanics, scheduled status reporting, rolling issue support, and PR marker/supersede helpersWhy
The previous pipeline depended on human-facing uv output, could not raise package caps, treated implementation as a stub, and could leave local or PR state inconsistent after failures. This change makes the workflow fail closed and gives promoted runs a tracked baseline that later runs can read.
Validation
env -u UV_EXCLUDE_NEWER -u UV_EXCLUDE_NEWER_PACKAGE uv run ruff check .env -u UV_EXCLUDE_NEWER -u UV_EXCLUDE_NEWER_PACKAGE uv run mypyenv -u UV_EXCLUDE_NEWER -u UV_EXCLUDE_NEWER_PACKAGE uv run pytest -q(280 passed, 3 skipped)env -u UV_EXCLUDE_NEWER -u UV_EXCLUDE_NEWER_PACKAGE uv run pytest -q --cov=agent_runtime_kit --cov-report=term-missing --cov-fail-under=85(89.56%)env -u UV_EXCLUDE_NEWER -u UV_EXCLUDE_NEWER_PACKAGE uv lock --check