fix(skillopt): local-review publish path + downgrade latch on definitive API rejections (#738)#740
Merged
Merged
Conversation
…ive rejections Closes #738 When a train session's human feedback is already imported locally (TrainingReady) before the review-publish step, `train continue` now records the review against the local markdown surface and advances to review_published without any GitHub call or watch registration, then lets the next hop take the existing TrainingReady -> feedback_synced path. Publish is a review SURFACE and the local markdown surface is already first-class for import, so a doomed GitHub issue (packet over GitHub's 64KB issue-body limit) is skipped entirely. When feedback is not yet ready, GitHub stays the default publish surface (byte-identical), including the June-validated publish-first / sync-skipped-after behavior. Also extends #735's posting_external marker downgrade so a definitive 4xx API rejection (HTTP 422 validation / 404 / 403) — which proves no issue was created — clears the latch and lets the retry proceed, while ambiguous failures (timeouts, kills, 5xx, socket drops) keep the conservative latch. Grounded in a new github.IsDefinitiveRejectionMessage classifier alongside IsTransientMessage. Part 3 (size-aware publish degradation) left as a code TODO referencing #738. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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
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 #738
Problem
A live training session wedged: its human feedback was already
TrainingReady(ranked + pairwise imported viafeedback markdown import), buttrain continuecould not reachreview_publishedwithout a successful GitHub post — and the post 422'd because the review packet exceeds GitHub's 65,536-char issue-body limit. Two root causes.1. Local-review path in the state machine
publishSkillOptTrainReviewnow checksloadSkillOptReviewStatusfor the iteration's eval run before attempting the GitHub post. Ifstatus.TrainingReadyis already true, it records the review against the local surface (metadata{status: published, review_surface: local}), registers no GitHub watch, transitions toreview_published, and returns aLocalSurfaceresult. The nexttrain continuehop takes the existingTrainingReady -> feedback_syncedpath.recoverSkillOptTrainReviewPublication, so it also unwedges a session left with a staleposting_externalmarker.published_externalrecovery marker (a real GitHub post that only needs a state-resume) is still honored via recover, not shadowed by a local record.2. Latch downgrade for definitive rejections
Extends #735's
downgradeReviewMarkerOnLaunchFailure: a definitive 4xx API rejection (HTTP 422 validation / 404 / 403) — which proves no issue was created — now also downgrades theposting_externalmarker (statusfailed_external_rejected) so the retry proceeds. Ambiguous failures (timeouts, kills, 5xx, socket drops) keep the conservative latch. Grounded in a newgithub.IsDefinitiveRejectionMessageclassifier that sits alongsideIsTransientMessage(disjoint 4xx vs 5xx/transport signature sets).3. Size-aware degradation (nice-to-have)
Left as a code
TODO(#738 part 3)at the publish site — non-trivial (summary issue + trailing comments/artifact). Parts 1+2 already resolve the wedge for the reported scenario.Tests (deterministic, no network)
TestSkillOptTrainContinueLocalReviewSkipsGitHub— TrainingReady-before-publish → continue skips GitHub (no client constructed), metadata carriesreview_surface: local, state →review_published, no watch registered.TestSkillOptTrainContinuePublishesToGitHubWhenFeedbackNotReady— not-ready → GitHub publish path invoked exactly as before (regression).TestDowngradeReviewMarkerOnDefinitiveRejection— 422/404/403 downgrade + retry proceeds; 5xx/timeout/killed/socket preserve the latch.TestSkillOptTrainContinueLocalReviewEndToEnd— full local path: seededoptions_generatedwith imported TrainingReady feedback → two continue hops reachfeedback_syncedwith zero GitHub calls.TestIsDefinitiveRejectionMessage(internal/github) — classifier unit coverage.Gate
go build ./...OKgo vet ./...OKgo test -count=1 ./internal/cli/OK (211.99s)go test -count=1 ./internal/github/OK;go test -race ./internal/github/OK-raceon the new cli tests OK🤖 Generated with Claude Code