diff --git a/.agents/skills/open-pr/SKILL.md b/.agents/skills/open-pr/SKILL.md new file mode 100644 index 00000000..5c56a6e0 --- /dev/null +++ b/.agents/skills/open-pr/SKILL.md @@ -0,0 +1,19 @@ +--- +name: open-pr +description: "Open the pull request for the current branch with a Boatstack-structured description, gated on a verified self-review attestation." +--- + + + +This adapter exposes the canonical Yield workflow at `skills/open-pr`. +Read its SKILL.md, then run from the repository root: + + .yield/bin/yskill run 'skills/open-pr' + + Follow each returned operation exactly. Answer each operation directly: + + .yield/bin/yskill respond --value --skill 'skills/open-pr' + + For structured agent results, use --result-json instead of --value. + +Do not skip an operation or invent its response. diff --git a/.agents/skills/self-review-solve/SKILL.md b/.agents/skills/self-review-solve/SKILL.md index a86bd2bb..221c6e28 100644 --- a/.agents/skills/self-review-solve/SKILL.md +++ b/.agents/skills/self-review-solve/SKILL.md @@ -3,7 +3,7 @@ name: self-review-solve description: "Resolve the Boatstack self-review: fix open findings or run a fresh review, converge the loop, and seal the receipt." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review-solve`. Read its SKILL.md, then run from the repository root: diff --git a/.agents/skills/self-review/SKILL.md b/.agents/skills/self-review/SKILL.md index b7100191..138e6b0c 100644 --- a/.agents/skills/self-review/SKILL.md +++ b/.agents/skills/self-review/SKILL.md @@ -3,7 +3,7 @@ name: self-review description: "Run the Boatstack supervisory-control self-review for the current branch and report the verdict without changing code." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review`. Read its SKILL.md, then run from the repository root: diff --git a/.claude/skills/open-pr/SKILL.md b/.claude/skills/open-pr/SKILL.md new file mode 100644 index 00000000..5c56a6e0 --- /dev/null +++ b/.claude/skills/open-pr/SKILL.md @@ -0,0 +1,19 @@ +--- +name: open-pr +description: "Open the pull request for the current branch with a Boatstack-structured description, gated on a verified self-review attestation." +--- + + + +This adapter exposes the canonical Yield workflow at `skills/open-pr`. +Read its SKILL.md, then run from the repository root: + + .yield/bin/yskill run 'skills/open-pr' + + Follow each returned operation exactly. Answer each operation directly: + + .yield/bin/yskill respond --value --skill 'skills/open-pr' + + For structured agent results, use --result-json instead of --value. + +Do not skip an operation or invent its response. diff --git a/.claude/skills/self-review-solve/SKILL.md b/.claude/skills/self-review-solve/SKILL.md index a86bd2bb..221c6e28 100644 --- a/.claude/skills/self-review-solve/SKILL.md +++ b/.claude/skills/self-review-solve/SKILL.md @@ -3,7 +3,7 @@ name: self-review-solve description: "Resolve the Boatstack self-review: fix open findings or run a fresh review, converge the loop, and seal the receipt." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review-solve`. Read its SKILL.md, then run from the repository root: diff --git a/.claude/skills/self-review/SKILL.md b/.claude/skills/self-review/SKILL.md index b7100191..138e6b0c 100644 --- a/.claude/skills/self-review/SKILL.md +++ b/.claude/skills/self-review/SKILL.md @@ -3,7 +3,7 @@ name: self-review description: "Run the Boatstack supervisory-control self-review for the current branch and report the verdict without changing code." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review`. Read its SKILL.md, then run from the repository root: diff --git a/.cursor/skills/open-pr/SKILL.md b/.cursor/skills/open-pr/SKILL.md new file mode 100644 index 00000000..5c56a6e0 --- /dev/null +++ b/.cursor/skills/open-pr/SKILL.md @@ -0,0 +1,19 @@ +--- +name: open-pr +description: "Open the pull request for the current branch with a Boatstack-structured description, gated on a verified self-review attestation." +--- + + + +This adapter exposes the canonical Yield workflow at `skills/open-pr`. +Read its SKILL.md, then run from the repository root: + + .yield/bin/yskill run 'skills/open-pr' + + Follow each returned operation exactly. Answer each operation directly: + + .yield/bin/yskill respond --value --skill 'skills/open-pr' + + For structured agent results, use --result-json instead of --value. + +Do not skip an operation or invent its response. diff --git a/.cursor/skills/self-review-solve/SKILL.md b/.cursor/skills/self-review-solve/SKILL.md index a86bd2bb..221c6e28 100644 --- a/.cursor/skills/self-review-solve/SKILL.md +++ b/.cursor/skills/self-review-solve/SKILL.md @@ -3,7 +3,7 @@ name: self-review-solve description: "Resolve the Boatstack self-review: fix open findings or run a fresh review, converge the loop, and seal the receipt." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review-solve`. Read its SKILL.md, then run from the repository root: diff --git a/.cursor/skills/self-review/SKILL.md b/.cursor/skills/self-review/SKILL.md index b7100191..138e6b0c 100644 --- a/.cursor/skills/self-review/SKILL.md +++ b/.cursor/skills/self-review/SKILL.md @@ -3,7 +3,7 @@ name: self-review description: "Run the Boatstack supervisory-control self-review for the current branch and report the verdict without changing code." --- - + This adapter exposes the canonical Yield workflow at `skills/self-review`. Read its SKILL.md, then run from the repository root: diff --git a/.github/codex/review-prompt.md b/.github/codex/review-prompt.md index 6327fbbd..acbc9ef9 100644 --- a/.github/codex/review-prompt.md +++ b/.github/codex/review-prompt.md @@ -16,6 +16,8 @@ Do not report: * architectural alternatives that are merely cleaner; * issues whose location or impact cannot be established from available evidence. +If you discover an important pre-existing issue near the changed surface, record it as a carried note inside `overall_explanation`, clearly labeled as pre-existing. Never emit it as a finding, and never let it affect the verdict. + The important review principle is: Local correctness does not imply control-system correctness. @@ -276,6 +278,24 @@ Boatstack kernel A repository control program must not be able to reach around the declared program interface and mutate kernel semantics directly. +## Durable-contract deltas + +Apply the deep checks in this section only when the diff itself changes a durable-contract surface: + +* a schema version, durable file format, or receipt/journal shape; +* an authority, capability, or identity rule; +* a recovery mapping or failure-to-transition routing; +* an admission, freshness, or compare-and-swap boundary. + +When the diff touches such a surface, check the deployed-world cases, not just the code's internal consistency: + +* migration from the schema/format that is actually deployed, not only from empty state; +* recovery reachability from states real deployments occupy, including states written by the previous version; +* authority bound to the exact issuance and exact target it was granted for; +* the window between resolve and apply under the changed freshness rule. + +When the diff does not touch such a surface, do not hunt these classes. A small change gets a small review scoped to what it changed. + ## Review to closure before reporting findings Do not return as soon as you find the first valid counterexample. @@ -388,13 +408,18 @@ Verdict: or * "patch is incorrect" +The verdict is decided by the blocking boundary, not by the presence of findings: + +* "patch is incorrect" is reserved for reviews with at least one blocking finding (priority P0 or P1); +* a review whose findings are all P2 or P3 returns "patch is correct" — those findings are residuals, recorded with the review but not blocking it. + Then provide: * a concise explanation; * confidence from 0 to 1; * whether model-level verification is recommended before merge. -"Patch is correct" means no actionable defect introduced by this change was established from the available evidence. +"Patch is correct" means no blocking defect introduced by this change was established from the available evidence; residual P2/P3 findings may still be listed. It does not mean global liveness or formal correctness has been proven. diff --git a/.github/reviews/convergence-boundary.receipt.json b/.github/reviews/convergence-boundary.receipt.json new file mode 100644 index 00000000..1d54eb69 --- /dev/null +++ b/.github/reviews/convergence-boundary.receipt.json @@ -0,0 +1,4 @@ +{ + "reviewed_tree": "638048ded4504ed3df431637e803348cc2100340", + "program_fingerprint": "ddbd77f1cbc842b3dffcb8e54b53ddcc624a998fb9c5a9ace15028580ab87967" +} diff --git a/.gitignore b/.gitignore index f9e4ad2a..4a928eac 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,9 @@ boatstack/boatstack-reviewer.exe .yield/ skills/*/.yield-runs/ skills/*/fixtures/tmp/ +skills/self-review/self-review +skills/self-review-solve/self-review-solve +skills/open-pr/open-pr dist/ build/docs/ node_modules/ diff --git a/boatstack/cmd/boatstack-reviewer/candidate.go b/boatstack/cmd/boatstack-reviewer/candidate.go index d69970cd..0e752882 100644 --- a/boatstack/cmd/boatstack-reviewer/candidate.go +++ b/boatstack/cmd/boatstack-reviewer/candidate.go @@ -51,14 +51,15 @@ type reviewAnchor struct { // observation from the exact staged bytes and the exact current diff; the // proposer's own claims are never trusted. type candidateSummary struct { - Fingerprint string `json:"fingerprint"` - ReviewedTree string `json:"reviewed_tree"` - Valid bool `json:"valid"` - InvalidReasons []string `json:"invalid_reasons,omitempty"` - Verdict string `json:"verdict,omitempty"` - Measure int `json:"measure"` - FindingCount int `json:"finding_count"` - Priorities [4]int `json:"priorities"` + Fingerprint string `json:"fingerprint"` + ReviewedTree string `json:"reviewed_tree"` + Valid bool `json:"valid"` + InvalidReasons []string `json:"invalid_reasons,omitempty"` + Verdict string `json:"verdict,omitempty"` + Measure int `json:"measure"` + BlockingMeasure int `json:"blocking_measure"` + FindingCount int `json:"finding_count"` + Priorities [4]int `json:"priorities"` } // candidateFingerprint identifies candidate review bytes by the sha256 of @@ -119,10 +120,21 @@ func evaluateCandidate(policy Policy, candidateBytes []byte, stagedTree, repoRoo summary.FindingCount = len(review.Findings) allowed := changedLines(diff) valid := true + // An incorrect verdict must be backed by at least one concrete finding; + // an empty assertion of incorrectness carries nothing the loop could + // act on and nothing the record could bind. + if review.OverallCorrectness == verdictIncorrect && len(review.Findings) == 0 { + valid = false + summary.InvalidReasons = append(summary.InvalidReasons, + "verdict rejects the patch but the review carries no findings") + } for index, finding := range review.Findings { if finding.Priority >= 0 && finding.Priority <= 3 { summary.Priorities[finding.Priority]++ summary.Measure += policy.Weights[finding.Priority] + if policy.Blocking[finding.Priority] { + summary.BlockingMeasure += policy.Weights[finding.Priority] + } } if reason := anchorFailure(finding.CodeLocation, repoRoot, allowed); reason != "" { valid = false @@ -264,9 +276,12 @@ func mustInt(value string) int { // stalled reports whether recording this candidate would extend the trailing // run of submissions without measure improvement to the policy stall window. -// The run length counts the submissions themselves: with a window of three, -// a third consecutive submission that fails to decrease the measure below -// its predecessor escalates instead of recording another round. +// The measures compared are BLOCKING measures: only progress on blocking +// findings counts, so residual P2/P3 churn can neither mask a stall nor +// trigger one. The run length counts the submissions themselves: with a +// window of three, a third consecutive submission that fails to decrease +// the blocking measure below its predecessor escalates instead of +// recording another round. func stalled(policy Policy, recorded []int, candidateMeasure int) bool { if len(recorded) == 0 { return false diff --git a/boatstack/cmd/boatstack-reviewer/domain.go b/boatstack/cmd/boatstack-reviewer/domain.go index c99ccf4e..e99098a5 100644 --- a/boatstack/cmd/boatstack-reviewer/domain.go +++ b/boatstack/cmd/boatstack-reviewer/domain.go @@ -26,10 +26,14 @@ type observationValue struct { Rounds []journalRound `json:"rounds"` } -func (v observationValue) roundMeasures() []int { +// roundBlockingMeasures projects the recorded rounds onto the convergence +// law's driving quantity: the blocking measure. Round and stall bounds are +// computed over this sequence, so residual (non-blocking) findings can +// neither extend nor cut short a review generation. +func (v observationValue) roundBlockingMeasures() []int { measures := make([]int, 0, len(v.Rounds)) for _, round := range v.Rounds { - measures = append(measures, round.Measure) + measures = append(measures, round.BlockingMeasure) } return measures } @@ -143,17 +147,35 @@ func submissionDisposition(policy Policy, observed observationValue) (string, st } return "", reasons } - if candidate.Verdict == verdictCorrect { - return transitionConverge, "candidate verdict accepts the patch" + // Convergence is deterministic on the blocking boundary: zero open + // blocking findings converges regardless of the verdict wording, and + // any open blocking finding keeps the loop running regardless of it. + // The verdict and residual (non-blocking) findings are recorded data. + if candidate.BlockingMeasure == 0 { + if residuals := residualCount(policy, candidate.Priorities); residuals > 0 { + return transitionConverge, fmt.Sprintf("no blocking findings remain; %d residual non-blocking findings are recorded", residuals) + } + return transitionConverge, "no blocking findings remain" } - rounds := observed.roundMeasures() + rounds := observed.roundBlockingMeasures() if len(rounds) >= policy.MaxRounds { return transitionEscalate, fmt.Sprintf("round bound %d is exhausted", policy.MaxRounds) } - if stalled(policy, rounds, candidate.Measure) { - return transitionEscalate, fmt.Sprintf("measure has not decreased for %d consecutive submissions", policy.StallWindow) + if stalled(policy, rounds, candidate.BlockingMeasure) { + return transitionEscalate, fmt.Sprintf("blocking measure has not decreased for %d consecutive submissions", policy.StallWindow) + } + return transitionRecord, "candidate records open blocking findings with a decreasing blocking measure" +} + +// residualCount counts findings in non-blocking priority classes. +func residualCount(policy Policy, priorities [4]int) int { + count := 0 + for priority, findings := range priorities { + if !policy.Blocking[priority] { + count += findings + } } - return transitionRecord, "candidate records open findings with a decreasing measure" + return count } // reviewOperator applies the one admitted operation. It receives only @@ -187,6 +209,7 @@ func (o reviewOperator) Execute(_ context.Context, operation kernel.Operation) ( MergeBase: observed.MergeBase, Verdict: observed.Candidate.Verdict, Measure: observed.Candidate.Measure, + BlockingMeasure: observed.Candidate.BlockingMeasure, FindingCount: observed.Candidate.FindingCount, Priorities: observed.Candidate.Priorities, Transition: operation.Transition.ID, @@ -252,6 +275,7 @@ func (d *reviewDomain) Verify(_ context.Context, evaluation kernel.Evaluation, e recorded.ReviewedTree != before.ReviewedTree || recorded.Verdict != before.Candidate.Verdict || recorded.Measure != before.Candidate.Measure || + recorded.BlockingMeasure != before.Candidate.BlockingMeasure || recorded.Transition != evaluation.Transition.ID { return fmt.Errorf("recorded round does not match the admitted candidate") } diff --git a/boatstack/cmd/boatstack-reviewer/main.go b/boatstack/cmd/boatstack-reviewer/main.go index ece02dc9..f8d9267d 100644 --- a/boatstack/cmd/boatstack-reviewer/main.go +++ b/boatstack/cmd/boatstack-reviewer/main.go @@ -83,7 +83,12 @@ func newLoopContext(repoPath, delivery, baseRef string) (*loopContext, error) { if err != nil { return nil, err } - policy, err := loadWorktreePolicy(repo.Root) + // The policy is admitted from the base revision — the same admission CI + // verification performs. A branch that changes the policy assets is + // therefore reviewed under the currently-admitted policy, and the + // changed policy governs only after it merges; otherwise the sealed + // attestation would carry a program fingerprint CI can never recompute. + policy, err := loadRevisionPolicy(repo, baseRef) if err != nil { return nil, err } @@ -152,6 +157,24 @@ type instructionsView struct { SchemaPath string `json:"output_schema_path"` SchemaSHA256 string `json:"output_schema_sha256"` SubmitCommand string `json:"submit_command"` + PolicyNote string `json:"policy_note,omitempty"` +} + +// worktreePolicyNote reports when the worktree policy assets differ from the +// base-admitted ones, so a proposer reading the worktree files knows which +// bytes govern the program identity and candidate validation. +func worktreePolicyNote(repoRoot string, admitted Policy) string { + for _, asset := range []struct{ path, admittedHash string }{ + {policyPromptPath, admitted.PromptSHA256}, + {policySchemaPath, admitted.SchemaSHA256}, + } { + contents, err := os.ReadFile(filepath.Join(repoRoot, filepath.FromSlash(asset.path))) + if err != nil || sha256Hex(contents) != asset.admittedHash { + return "the worktree policy assets differ from the base-admitted policy; " + + "the admitted (base revision) assets govern the program identity and candidate validation" + } + } + return "" } func commandResolve(arguments []string) error { @@ -215,6 +238,7 @@ func commandResolve(arguments []string) error { SchemaPath: policySchemaPath, SchemaSHA256: loop.policy.SchemaSHA256, SubmitCommand: "boatstack-reviewer submit --findings --actor ", + PolicyNote: worktreePolicyNote(loop.repo.Root, loop.policy), }, }) } @@ -302,9 +326,9 @@ func commandSubmit(arguments []string) error { func submissionGuidance(mode string) string { switch mode { case modeConverged: - return "review converged; run `boatstack-reviewer seal` and commit the sealed receipt" + return "review converged; run `boatstack-reviewer seal` and commit the sealed attestation" case modeFindingsOpen: - return "findings are open; fix them, commit, and submit a fresh review of the new tree" + return "blocking findings are open; fix them, commit, and submit a fresh review of the new tree" case modeEscalated: return "the loop escalated; a human must decide, then `boatstack-reviewer reopen --actor `" default: diff --git a/boatstack/cmd/boatstack-reviewer/policy.go b/boatstack/cmd/boatstack-reviewer/policy.go index ce6dc140..34205d06 100644 --- a/boatstack/cmd/boatstack-reviewer/policy.go +++ b/boatstack/cmd/boatstack-reviewer/policy.go @@ -5,8 +5,6 @@ import ( "encoding/hex" "encoding/json" "fmt" - "os" - "path/filepath" ) // The admitted review policy is the pair of repository assets the previous @@ -34,14 +32,23 @@ const ( // convergence measure V = sum(weight(priority)) over open findings. var defaultWeights = [4]int{1000, 100, 10, 1} +// defaultBlocking marks which priorities block convergence. P0 and P1 — +// control-law violations — must reach zero; P2 and P3 are recorded as +// residuals and never drive another round. Mined history: two thirds of +// past P1 findings were durable-contract defects (migration, recovery +// reachability, authority binding, freshness) that must not ship, while +// P2/P3 churn is exactly what caused long non-converging review loops. +var defaultBlocking = [4]bool{true, true, false, false} + type Policy struct { - PromptPath string `json:"prompt_path"` - PromptSHA256 string `json:"prompt_sha256"` - SchemaPath string `json:"schema_path"` - SchemaSHA256 string `json:"schema_sha256"` - MaxRounds int `json:"max_rounds"` - StallWindow int `json:"stall_window"` - Weights [4]int `json:"weights"` + PromptPath string `json:"prompt_path"` + PromptSHA256 string `json:"prompt_sha256"` + SchemaPath string `json:"schema_path"` + SchemaSHA256 string `json:"schema_sha256"` + MaxRounds int `json:"max_rounds"` + StallWindow int `json:"stall_window"` + Weights [4]int `json:"weights"` + Blocking [4]bool `json:"blocking"` PromptBytes []byte `json:"-"` SchemaBytes []byte `json:"-"` @@ -61,30 +68,18 @@ func newPolicy(promptBytes, schemaBytes []byte) Policy { MaxRounds: defaultMaxRounds, StallWindow: defaultStallWindow, Weights: defaultWeights, + Blocking: defaultBlocking, PromptBytes: promptBytes, SchemaBytes: schemaBytes, } } -// loadWorktreePolicy reads the policy assets from the repository worktree. -// The local loop always reviews under the policy present in the tree being -// reviewed; CI verification separately re-admits the policy from the pull -// request base revision. -func loadWorktreePolicy(repoRoot string) (Policy, error) { - prompt, err := os.ReadFile(filepath.Join(repoRoot, filepath.FromSlash(policyPromptPath))) - if err != nil { - return Policy{}, fmt.Errorf("review policy prompt is unavailable: %w", err) - } - schema, err := os.ReadFile(filepath.Join(repoRoot, filepath.FromSlash(policySchemaPath))) - if err != nil { - return Policy{}, fmt.Errorf("review policy schema is unavailable: %w", err) - } - return newPolicy(prompt, schema), nil -} - // loadRevisionPolicy reads the policy assets from an exact committed // revision, mirroring the base-revision admission the retired CI reviewer -// performed with `git show "$BASE_SHA:"`. +// performed with `git show "$BASE_SHA:"`. Both the local loop and CI +// verification admit from the base revision, so a branch that changes the +// policy assets is reviewed under the currently-admitted policy and the +// change takes effect after merge. func loadRevisionPolicy(repo *gitRepo, revision string) (Policy, error) { prompt, err := repo.showFile(revision, policyPromptPath) if err != nil { @@ -103,14 +98,15 @@ func loadRevisionPolicy(repo *gitRepo, revision string) (Policy, error) { // by the kernel Program fingerprint. func (p Policy) contractFingerprint() (string, error) { encoded, err := json.Marshal(struct { - PromptPath string `json:"prompt_path"` - PromptSHA256 string `json:"prompt_sha256"` - SchemaPath string `json:"schema_path"` - SchemaSHA256 string `json:"schema_sha256"` - MaxRounds int `json:"max_rounds"` - StallWindow int `json:"stall_window"` - Weights [4]int `json:"weights"` - }{p.PromptPath, p.PromptSHA256, p.SchemaPath, p.SchemaSHA256, p.MaxRounds, p.StallWindow, p.Weights}) + PromptPath string `json:"prompt_path"` + PromptSHA256 string `json:"prompt_sha256"` + SchemaPath string `json:"schema_path"` + SchemaSHA256 string `json:"schema_sha256"` + MaxRounds int `json:"max_rounds"` + StallWindow int `json:"stall_window"` + Weights [4]int `json:"weights"` + Blocking [4]bool `json:"blocking"` + }{p.PromptPath, p.PromptSHA256, p.SchemaPath, p.SchemaSHA256, p.MaxRounds, p.StallWindow, p.Weights, p.Blocking}) if err != nil { return "", err } @@ -132,5 +128,12 @@ func (p Policy) validate() error { return fmt.Errorf("review policy requires positive priority weights") } } + anyBlocking := false + for _, blocking := range p.Blocking { + anyBlocking = anyBlocking || blocking + } + if !anyBlocking { + return fmt.Errorf("review policy requires at least one blocking priority") + } return nil } diff --git a/boatstack/cmd/boatstack-reviewer/reviewer_test.go b/boatstack/cmd/boatstack-reviewer/reviewer_test.go index b8c97fc2..50d98403 100644 --- a/boatstack/cmd/boatstack-reviewer/reviewer_test.go +++ b/boatstack/cmd/boatstack-reviewer/reviewer_test.go @@ -113,29 +113,28 @@ func newTestLoop(t *testing.T, scratch *scratchRepo, policy Policy) *loopContext func testPolicy(t *testing.T, scratch *scratchRepo) Policy { t.Helper() - policy, err := loadWorktreePolicy(scratch.repo.Root) + policy, err := loadRevisionPolicy(scratch.repo, "main") if err != nil { t.Fatal(err) } return policy } +func reviewWith(verdict string, findings ...string) string { + return fmt.Sprintf(`{ + "findings": [%s], + "overall_correctness": %q, + "overall_explanation": "Explanation of the verdict.", + "overall_confidence_score": 0.9 +}`, strings.Join(findings, ","), verdict) +} + func correctReview() string { - return `{ - "findings": [], - "overall_correctness": "patch is correct", - "overall_explanation": "No remaining actionable findings.", - "overall_confidence_score": 0.95 -}` + return reviewWith(verdictCorrect) } func incorrectReview(findings ...string) string { - return fmt.Sprintf(`{ - "findings": [%s], - "overall_correctness": "patch is incorrect", - "overall_explanation": "Actionable findings remain.", - "overall_confidence_score": 0.9 -}`, strings.Join(findings, ",")) + return reviewWith(verdictIncorrect, findings...) } func finding(title string, priority int, path string, line int) string { @@ -227,6 +226,24 @@ func TestReviewProgramControlLaw(t *testing.T) { if reprogram.Fingerprint == program.Fingerprint { t.Fatal("changing the convergence bound did not change the program identity") } + // The blocking boundary is part of the convergence law: moving it must + // also move the program identity. + reblocked := policy + reblocked.Blocking = [4]bool{true, true, true, false} + reblockedProgram, err := compileReviewProgram(reblocked) + if err != nil { + t.Fatal(err) + } + if reblockedProgram.Fingerprint == program.Fingerprint { + t.Fatal("changing the blocking boundary did not change the program identity") + } + // A policy where nothing blocks would converge on any valid candidate; + // it must not validate. + unblocked := policy + unblocked.Blocking = [4]bool{} + if err := unblocked.validate(); err == nil { + t.Fatal("a policy with no blocking priority validated") + } } func TestCalibrationCoversMinedReviewHistory(t *testing.T) { @@ -341,6 +358,135 @@ func TestStallLaw(t *testing.T) { } } +func TestConvergesWithResidualFindingsUnderEitherVerdict(t *testing.T) { + // The blocking boundary decides convergence, not the verdict wording: + // a review carrying only P2/P3 findings converges even when the + // proposer wrote "patch is incorrect", and the residuals are recorded + // with the round and travel into the sealed receipt. + scratch := newScratchRepo(t) + policy := testPolicy(t, scratch) + loop := newTestLoop(t, scratch, policy) + + residuals := reviewWith(verdictIncorrect, + finding("residual P2", 2, "subject.go", 3), + finding("residual P3", 3, "subject.go", 3)) + _, receipt, err := submit(t, loop, residuals) + if err != nil { + t.Fatal(err) + } + if receipt.TransitionID != transitionConverge { + t.Fatalf("a residual-only review committed %q, not convergence", receipt.TransitionID) + } + if mode(t, loop) != modeConverged { + t.Fatalf("mode is %q after residual-only convergence", mode(t, loop)) + } + + sealed, err := buildSealedReceipt(scratch.repo, loop.store, policy, loop.program, "main", time.Now()) + if err != nil { + t.Fatal(err) + } + if report := verifyFullReceipt(scratch.repo, sealed, "", "main", "HEAD"); !report.Verified { + t.Fatalf("residual-only converged receipt did not verify at seal time: %v", report.Failures) + } + final := sealed.Rounds[len(sealed.Rounds)-1] + if final.BlockingMeasure != 0 { + t.Fatalf("converged round records blocking measure %d", final.BlockingMeasure) + } + if final.Priorities != [4]int{0, 0, 1, 1} { + t.Fatalf("residual priorities are not recorded in the sealed round: %v", final.Priorities) + } + if final.Measure != policy.Weights[2]+policy.Weights[3] { + t.Fatalf("total measure %d does not carry the residual weights", final.Measure) + } +} + +func TestBlockingFindingRefusesConvergenceDespiteCorrectVerdict(t *testing.T) { + // The inverse of the residual case: an open P1 keeps the loop running + // even when the proposer wrote "patch is correct". The verdict wording + // cannot converge past a blocking finding. + scratch := newScratchRepo(t) + policy := testPolicy(t, scratch) + loop := newTestLoop(t, scratch, policy) + + optimistic := reviewWith(verdictCorrect, finding("open P1", 1, "subject.go", 3)) + _, receipt, err := submit(t, loop, optimistic) + if err != nil { + t.Fatal(err) + } + if receipt.TransitionID != transitionRecord { + t.Fatalf("a review with an open P1 committed %q, not a recorded round", receipt.TransitionID) + } + if mode(t, loop) != modeFindingsOpen { + t.Fatalf("mode is %q with a blocking finding open", mode(t, loop)) + } +} + +func TestIncorrectVerdictWithoutFindingsIsInvalid(t *testing.T) { + // An empty assertion of incorrectness carries nothing to act on and + // nothing to bind; it must not become a recorded round. + scratch := newScratchRepo(t) + policy := testPolicy(t, scratch) + loop := newTestLoop(t, scratch, policy) + + head := scratch.git("rev-parse", "HEAD") + mergeBase := scratch.git("merge-base", "main", "HEAD") + diff, err := scratch.repo.pullRequestDiff(mergeBase, head) + if err != nil { + t.Fatal(err) + } + tree, err := scratch.repo.reviewedTree(head) + if err != nil { + t.Fatal(err) + } + summary := evaluateCandidate(policy, []byte(incorrectReview()), tree, scratch.repo.Root, diff) + if summary.Valid { + t.Fatal("an incorrect verdict with zero findings was admitted") + } + + if _, _, err := submit(t, loop, incorrectReview()); err == nil { + t.Fatal("an incorrect verdict with zero findings was prescribed") + } + if mode(t, loop) != modeUnreviewed { + t.Fatalf("a refused candidate moved the mode to %q", mode(t, loop)) + } +} + +func TestResidualChurnCannotMaskABlockingStall(t *testing.T) { + // The stall law runs over the blocking measure. Three submissions keep + // the same P1 open while the P2 count shrinks each round: the total + // measure decreases, but the blocking measure is flat, so the third + // submission escalates instead of recording another round. + scratch := newScratchRepo(t) + policy := testPolicy(t, scratch) + loop := newTestLoop(t, scratch, policy) + + rounds := []string{ + reviewWith(verdictIncorrect, + finding("stuck P1", 1, "subject.go", 3), + finding("nit one", 2, "subject.go", 3), + finding("nit two", 2, "subject.go", 3)), + reviewWith(verdictIncorrect, + finding("stuck P1", 1, "subject.go", 3), + finding("nit one", 2, "subject.go", 3)), + reviewWith(verdictIncorrect, + finding("stuck P1", 1, "subject.go", 3)), + } + var last *kernel.Receipt + for _, candidate := range rounds { + _, receipt, err := submit(t, loop, candidate) + if err != nil { + t.Fatal(err) + } + last = receipt + } + if last.TransitionID != transitionEscalate { + t.Fatalf("a flat blocking measure behind shrinking residuals committed %q, not escalation", last.TransitionID) + } + if mode(t, loop) != modeEscalated { + t.Fatalf("mode is %q after the blocking stall", mode(t, loop)) + } +} + func TestLoopConvergesSealsAndVerifies(t *testing.T) { scratch := newScratchRepo(t) policy := testPolicy(t, scratch) @@ -650,6 +796,67 @@ func TestVerificationRejectsTamperingForgeryAndBypass(t *testing.T) { } } +func TestPolicyChangingBranchSealsAVerifiableAttestation(t *testing.T) { + // Regression for the policy-admission split: a branch that changes the + // admitted policy assets must still seal an attestation CI can verify. + // The loop admits the policy from the base revision — the same admission + // verification performs — so the changed prompt governs only after + // merge, and the sealed fingerprint recomputes identically from base. + scratch := newScratchRepo(t) + scratch.writeFile(policyPromptPath, "an entirely rewritten review policy for the next generation\n") + scratch.commitAll("change the review policy on the branch") + + policy := testPolicy(t, scratch) + worktreePrompt, err := os.ReadFile(filepath.Join(scratch.repo.Root, filepath.FromSlash(policyPromptPath))) + if err != nil { + t.Fatal(err) + } + if sha256Hex(worktreePrompt) == policy.PromptSHA256 { + t.Fatal("the fixture did not actually diverge the worktree prompt from the base-admitted prompt") + } + if note := worktreePolicyNote(scratch.repo.Root, policy); note == "" { + t.Fatal("the resolve instructions do not surface the policy drift") + } + + loop := newTestLoop(t, scratch, policy) + if _, receipt, err := submit(t, loop, correctReview()); err != nil { + t.Fatal(err) + } else if receipt.TransitionID != transitionConverge { + t.Fatalf("the policy-changing branch committed %q", receipt.TransitionID) + } + sealed, err := buildSealedReceipt(scratch.repo, loop.store, policy, loop.program, "main", time.Now()) + if err != nil { + t.Fatal(err) + } + if report := verifyFullReceipt(scratch.repo, sealed, "", "main", "HEAD"); !report.Verified { + t.Fatalf("the full receipt did not verify at seal time: %v", report.Failures) + } + path := filepath.Join(scratch.repo.receiptDirectoryPath(), "feature.receipt.json") + if err := writeAttestation(path, attestationOf(sealed)); err != nil { + t.Fatal(err) + } + scratch.commitAll("sealed review attestation") + if report := verifyAttestation(scratch.repo, attestationOf(sealed), path, "main", "HEAD"); !report.Verified { + t.Fatalf("the attestation of a policy-changing branch did not verify against the base admission: %v", report.Failures) + } + + // The inverse remains refused: an attestation fingerprinted under the + // worktree (changed) policy is not the base-admitted program. + worktreeSchema, err := os.ReadFile(filepath.Join(scratch.repo.Root, filepath.FromSlash(policySchemaPath))) + if err != nil { + t.Fatal(err) + } + worktreeProgram, err := compileReviewProgram(newPolicy(worktreePrompt, worktreeSchema)) + if err != nil { + t.Fatal(err) + } + forged := attestationOf(sealed) + forged.ProgramFingerprint = worktreeProgram.Fingerprint + if report := verifyAttestation(scratch.repo, forged, path, "main", "HEAD"); report.Verified { + t.Fatal("an attestation fingerprinted under the unadmitted worktree policy verified") + } +} + func TestVerificationRejectsReceiptDeclaredBoundsDrift(t *testing.T) { // Regression for round 1, finding 1: a receipt sealed under weakened // convergence bounds must not verify against the admitted policy — @@ -814,7 +1021,7 @@ func TestShowDisplaysTheRecordedReviewWithoutResolving(t *testing.T) { } original := os.Stdout os.Stdout = writer - runErr := run(append([]string{"show", "--repo", scratch.repo.Root, "--delivery", "feature"}, arguments...)) + runErr := run(append([]string{"show", "--repo", scratch.repo.Root, "--delivery", "feature", "--base", "main"}, arguments...)) os.Stdout = original writer.Close() output, err := io.ReadAll(reader) diff --git a/boatstack/cmd/boatstack-reviewer/seal.go b/boatstack/cmd/boatstack-reviewer/seal.go index 1cfbc0cf..85f5b4e8 100644 --- a/boatstack/cmd/boatstack-reviewer/seal.go +++ b/boatstack/cmd/boatstack-reviewer/seal.go @@ -99,8 +99,8 @@ func buildSealedReceipt(repo *gitRepo, store *fileStore, policy Policy, program return SealedReceipt{}, fmt.Errorf("converged instance has no recorded rounds") } final := rounds[len(rounds)-1] - if final.Verdict != verdictCorrect || final.Transition != transitionConverge { - return SealedReceipt{}, fmt.Errorf("last recorded round is not the converged round") + if final.Transition != transitionConverge || final.BlockingMeasure != 0 { + return SealedReceipt{}, fmt.Errorf("last recorded round is not a converged round with zero blocking findings") } head, err := repo.headCommit() if err != nil { @@ -333,12 +333,13 @@ func verifyFullReceipt(repo *gitRepo, receipt SealedReceipt, receiptPath, baseRe admittedPolicy := basePolicy if receipt.Policy.MaxRounds != admittedPolicy.MaxRounds || receipt.Policy.StallWindow != admittedPolicy.StallWindow || - receipt.Policy.Weights != admittedPolicy.Weights { - fail("sealed receipt declares convergence bounds (rounds %d, stall %d, weights %v) that differ from the admitted policy (rounds %d, stall %d, weights %v)", - receipt.Policy.MaxRounds, receipt.Policy.StallWindow, receipt.Policy.Weights, - admittedPolicy.MaxRounds, admittedPolicy.StallWindow, admittedPolicy.Weights) + receipt.Policy.Weights != admittedPolicy.Weights || + receipt.Policy.Blocking != admittedPolicy.Blocking { + fail("sealed receipt declares convergence bounds (rounds %d, stall %d, weights %v, blocking %v) that differ from the admitted policy (rounds %d, stall %d, weights %v, blocking %v)", + receipt.Policy.MaxRounds, receipt.Policy.StallWindow, receipt.Policy.Weights, receipt.Policy.Blocking, + admittedPolicy.MaxRounds, admittedPolicy.StallWindow, admittedPolicy.Weights, admittedPolicy.Blocking) } else { - pass("convergence bounds and weights match the admitted policy") + pass("convergence bounds, weights, and blocking boundary match the admitted policy") } program, err := compileReviewProgram(admittedPolicy) if err != nil { @@ -441,8 +442,8 @@ func verifyFullReceipt(repo *gitRepo, receipt SealedReceipt, receiptPath, baseRe return report } finalRound := receipt.Rounds[len(receipt.Rounds)-1] - if finalRound.Transition != transitionConverge || finalRound.Verdict != verdictCorrect { - fail("the final recorded round is not a convergence") + if finalRound.Transition != transitionConverge || finalRound.BlockingMeasure != 0 { + fail("the final recorded round is not a convergence with zero blocking findings") } if finalRound.ReviewedTree != receipt.ReviewedTree { fail("the converged round bound tree %s, not the sealed tree %s", finalRound.ReviewedTree, receipt.ReviewedTree) @@ -467,10 +468,10 @@ func verifyFullReceipt(repo *gitRepo, receipt SealedReceipt, receiptPath, baseRe summary := evaluateCandidate(admittedPolicy, receipt.FinalReview, receipt.ReviewedTree, repo.Root, diff) if !summary.Valid && diffErr == nil { fail("final review does not revalidate against the admitted schema and diff: %s", strings.Join(summary.InvalidReasons, "; ")) - } else if summary.Verdict != verdictCorrect { - fail("final review verdict is %q, not %q", summary.Verdict, verdictCorrect) + } else if summary.BlockingMeasure != 0 { + fail("final review leaves blocking findings open (blocking measure %d recomputed from the exact bytes)", summary.BlockingMeasure) } else { - pass("final review bytes revalidate and accept the patch") + pass("final review bytes revalidate with zero blocking findings") } } diff --git a/boatstack/cmd/boatstack-reviewer/store.go b/boatstack/cmd/boatstack-reviewer/store.go index fde5cd93..560afc1c 100644 --- a/boatstack/cmd/boatstack-reviewer/store.go +++ b/boatstack/cmd/boatstack-reviewer/store.go @@ -42,6 +42,7 @@ type journalRound struct { MergeBase string `json:"merge_base"` Verdict string `json:"verdict"` Measure int `json:"measure"` + BlockingMeasure int `json:"blocking_measure"` FindingCount int `json:"finding_count"` Priorities [4]int `json:"priorities"` Transition string `json:"transition"` diff --git a/docs/self-review.md b/docs/self-review.md index be3406ef..227ee130 100644 --- a/docs/self-review.md +++ b/docs/self-review.md @@ -23,15 +23,29 @@ resolve/apply relation. - **Modes:** `unreviewed` → `findings-open` → `converged` (the only marked mode), with `escalated` for bounded non-convergence. - **Admitted policy:** `.github/codex/review-prompt.md` (review instructions) - and `.github/codex/review-output-schema.json` (output contract). Their exact - bytes, the round bound, the stall window, and the priority weights are - hashed into the program fingerprint; any change stales every prior - prescription and receipt. + and `.github/codex/review-output-schema.json` (output contract), admitted + from the pull request base revision — by the local loop and by CI alike. + A branch that changes the policy assets is therefore reviewed under the + currently-admitted policy, and the change governs only after merge; + `resolve` surfaces a policy note when the worktree assets have drifted + from the admitted ones. The exact admitted bytes, the round bound, the + stall window, the priority weights, and the blocking boundary are hashed + into the program fingerprint; any change stales every prior prescription + and receipt. +- **Blocking boundary:** priorities P0 and P1 block; P2 and P3 are + residuals. Convergence is deterministic on this boundary, not on the + verdict wording: a candidate converges exactly when its blocking measure + is zero, and an open blocking finding keeps the loop running even under a + "patch is correct" verdict. Residual findings are recorded with the round + (and travel into the sealed receipt's round record) but never demand + another round. A "patch is incorrect" verdict with zero findings is an + invalid candidate. - **Convergence measure:** each finding weighs by priority (P0 1000, P1 100, - P2 10, P3 1). A submission that fails to decrease the measure extends a - stall run; the loop escalates on the third consecutive non-improving - submission or after sixteen rounds in one generation. The bounds are - calibrated against mined review history + P2 10, P3 1). The stall and round bounds run over the blocking measure + only, so residual churn can neither drive rounds nor trigger escalation: + the loop escalates on the third consecutive submission that fails to + decrease the blocking measure, or after sixteen rounds in one generation. + The bounds are calibrated against mined review history (`boatstack/cmd/boatstack-reviewer/testdata/review_rounds.json`). - **Freshness:** every submission binds the exact committed tree it reviewed. A dirty worktree, a new commit, or an edited candidate refuses admission @@ -50,9 +64,10 @@ Work on a branch, commit your change, then: reviewer validates the candidate (schema, diff anchors, tree binding) and the kernel commits one transition: findings recorded, converged, or escalated. Refusals name their reason and record nothing. -3. Fix the recorded findings, commit, and repeat. The measure must trend - down; convergence requires a fresh review of the fixed tree whose verdict - is `patch is correct`. +3. Fix the recorded blocking findings, commit, and repeat. The blocking + measure must trend down; convergence requires a fresh review of the fixed + tree with no open P0/P1 finding. Residual P2/P3 findings do not need to + be fixed for convergence — they are recorded for the user to weigh. 4. `boatstack-reviewer seal` — verifies the full receipt (round trajectory, kernel receipt chain, final review bytes) locally, archives it in the local store, and writes only a minimal attestation to @@ -78,14 +93,24 @@ for Cursor, Codex, and Claude Code under their skill directories): - `skills/self-review` — report-only: builds the reviewer from the current tree, resolves the control state, has the agent review exactly the admitted range under the admitted schema, submits, and reports the - recorded verdict in the conversation. It changes no code, verifies + recorded verdict in the conversation — plus the titles of any residual + P2/P3 findings when the round converged. It changes no code, verifies afterwards that no tracked file changed, and never seals, commits, or pushes. -- `skills/self-review-solve` — drives to convergence: fixes open findings in - code (committed by the agent), re-reviews the fixed tree, repeats within a - bounded attempt budget, then seals and commits the minimal attestation - locally. It never pushes; pushing is the user's decision. An escalated - loop asks the human before reopening. +- `skills/self-review-solve` — drives to convergence: fixes open blocking + (P0/P1) findings in code (committed by the agent), re-reviews the fixed + tree, repeats within a bounded attempt budget, then seals and commits the + minimal attestation locally. Residual P2/P3 findings are never fixed by + the loop; they are listed in the completion payload for the user to + decide about. It never pushes; pushing is the user's decision. An + escalated loop asks the human before reopening. +- `skills/open-pr` — opens the pull request for the current branch behind + the same gate CI enforces: it refuses unless the committed attestation + verifies for the exact head tree, so running it is the explicit decision + to push and open a PR for a converged, sealed head. The description + follows a fixed structure — agent-drafted Boundary, Transition, and + Evidence sections plus workflow-gathered facts: the commit list, the + attestation binding, and the converged review's residual findings. Run either with `.yield/bin/yskill run 'skills/'` from the repository root. `yskill doctor 'skills/' --test` exercises each workflow against @@ -99,7 +124,14 @@ fingerprint, recomputes the receipt-excluded head tree, and checks that the committed attestation names exactly those two facts. Nothing else travels with the pull request: the attestation is deliberately minimal because the program fingerprint already hashes the prompt bytes, the schema bytes, the -round bound, the stall window, and the priority weights. +round bound, the stall window, the priority weights, and the blocking +boundary. + +Changing any admitted policy asset or bound produces a new program +fingerprint, so existing local review instances become stale. That is the +expected one-time step after such a change: `boatstack-reviewer reset +--confirm` archives the stale instance, and the next review starts fresh +under the new program identity. ## What the attestation does and does not prove diff --git a/release-notes/2026-08-22-blocking-convergence-boundary.md b/release-notes/2026-08-22-blocking-convergence-boundary.md new file mode 100644 index 00000000..9594e4e4 --- /dev/null +++ b/release-notes/2026-08-22-blocking-convergence-boundary.md @@ -0,0 +1,3 @@ +### Deterministic blocking boundary for review convergence + +Review convergence is now decided by a deterministic priority law instead of the verdict wording: P0/P1 findings block, P2/P3 findings are residuals. A candidate converges exactly when its blocking measure is zero — an open P1 keeps the loop running even under a "patch is correct" verdict, and residual-only reviews converge with the residuals recorded in the round record and sealed receipt. The stall and round bounds run over the blocking measure only, so residual churn can neither drive rounds nor trigger escalation, and a "patch is incorrect" verdict with zero findings is refused as an invalid candidate. The blocking boundary is hashed into the program fingerprint. The admitted review prompt scopes the deep durable-contract checks (migration from deployed state, recovery reachability, authority binding, resolve-to-apply freshness) to diffs that actually touch such a surface, and nearby pre-existing issues become carried notes in the overall explanation rather than findings. The self-review skill reports residual titles at convergence; self-review-solve fixes blocking findings only and lists residuals for the user to decide. The local loop now admits the policy from the base revision — the same admission CI verification performs — so a branch that changes the policy assets is reviewed under the currently-admitted policy and still seals an attestation CI can verify; the changed policy governs after merge. Existing local review instances become stale under the new program fingerprint; `boatstack-reviewer reset --confirm` archives them. diff --git a/release-notes/2026-08-22-open-pr-skill.md b/release-notes/2026-08-22-open-pr-skill.md new file mode 100644 index 00000000..0c6493ef --- /dev/null +++ b/release-notes/2026-08-22-open-pr-skill.md @@ -0,0 +1,3 @@ +### Pull requests open through the review gate + +A new Yield workflow, `skills/open-pr`, opens the pull request for the current branch behind the same deterministic gate CI enforces: it refuses unless the committed review attestation verifies for the exact head tree, so a pull request can only be opened for a head whose self-review converged and was sealed. The description follows a fixed Boatstack structure — agent-drafted Boundary, Transition, and Evidence sections, plus facts the workflow gathers itself: the branch commit list, the attestation binding (reviewed tree and program fingerprint), and any residual non-blocking findings the converged review recorded. Running the skill is the explicit decision to push and open the pull request; every refused gate leaves the remote untouched. Fixture tests drive the whole flow against a scratch repository with a local bare remote and a stub `gh`, so testing never pushes anywhere real. diff --git a/skills/open-pr/SKILL.md b/skills/open-pr/SKILL.md new file mode 100644 index 00000000..8e527f20 --- /dev/null +++ b/skills/open-pr/SKILL.md @@ -0,0 +1,36 @@ +--- +name: open-pr +description: "Open the pull request for the current branch with a Boatstack-structured description, gated on a verified self-review attestation." +--- + +Open the pull request for the current branch. This is a governed boundary: +the workflow refuses unless the committed review attestation verifies for +the exact head tree — the same deterministic check CI performs — so a pull +request only opens for a head whose self-review converged and was sealed. +Run `skills/self-review-solve` first when the gate refuses. + +The description follows a fixed structure. The agent drafts the prose of +three sections — Boundary (the durable control boundary the change +crosses), Transition (refused/allowed behavior before versus after), and +Evidence (what was run and what it proves) — and the workflow appends the +facts it gathers itself: the commit list, the attestation binding (reviewed +tree and program fingerprint), and any residual P2/P3 findings the +converged review recorded. Running this skill is the explicit decision to +push the branch and create the pull request; nothing is pushed when any +gate refuses. + +Run from the repository root: + + .yield/bin/yskill run 'skills/open-pr' + +If `.yield/bin/yskill` is missing, install the pinned runtime first: + + go install github.com/operatorstack/yield/cmd/yskill@v0.1.38 && yskill init skills/open-pr --language go + +Follow each returned operation exactly. Answer it directly: + + .yield/bin/yskill respond --result-json --skill 'skills/open-pr' + +Do not skip an operation or invent a response. When the run asks for the +draft (the `draft` agent task), write the three sections for a reviewer who +has not followed the work, and return only the schema-valid JSON object. diff --git a/skills/open-pr/fixtures/responses.json b/skills/open-pr/fixtures/responses.json new file mode 100644 index 00000000..e89e3234 --- /dev/null +++ b/skills/open-pr/fixtures/responses.json @@ -0,0 +1,8 @@ +{ + "draft": { + "title": "Add Value accessor to the subject package", + "boundary": "The scratch subject package gains its first public accessor, crossing the boundary between an empty placeholder and a consumed API surface.", + "transition": "Before, callers had no way to read a value from the subject package; after, Value() returns the fixed integer the fixture change introduces.", + "evidence": "The self-review converged with zero findings over the exact range, and the sealed attestation binds the reviewed tree deterministically." + } +} diff --git a/skills/open-pr/fixtures/setup.sh b/skills/open-pr/fixtures/setup.sh new file mode 100644 index 00000000..f09069d7 --- /dev/null +++ b/skills/open-pr/fixtures/setup.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# Test-only fixture: build a scratch repository with a local bare remote, a +# sealed review attestation, and a stub `gh`, so the workflow's real commands +# never push anywhere real and never open a real pull request. Runs with the +# skill directory as the working directory and YIELD_FIXTURE=1. +set -eu + +root="$(git rev-parse --show-toplevel)" +scratch="$PWD/fixtures/tmp/repo" +rm -rf "$PWD/fixtures/tmp" +mkdir -p "$scratch" + +git -C "$scratch" init -q -b main +git -C "$scratch" config user.email "open-pr-fixture@example.invalid" +git -C "$scratch" config user.name "Open PR Fixture" + +mkdir -p "$scratch/.github/codex" +cp "$root/.github/codex/review-prompt.md" "$scratch/.github/codex/" +cp "$root/.github/codex/review-output-schema.json" "$scratch/.github/codex/" +printf 'package subject\n' > "$scratch/subject.go" +git -C "$scratch" add -A +git -C "$scratch" commit -qm "base" + +git -C "$scratch" checkout -qb feature +printf 'package subject\n\nfunc Value() int { return 1 }\n' > "$scratch/subject.go" +git -C "$scratch" commit -qam "change under review" + +# A local bare remote so the workflow's real `git push` has somewhere safe. +git init -q --bare "$PWD/fixtures/tmp/origin.git" +git -C "$scratch" remote add origin "$PWD/fixtures/tmp/origin.git" + +# Drive the real reviewer to a sealed, committed attestation so the +# workflow's verification gate passes for the scratch head. +reviewer="$PWD/fixtures/tmp/boatstack-reviewer" +go build -C "$root/boatstack" -o "$reviewer" ./cmd/boatstack-reviewer +cat > "$PWD/fixtures/tmp/correct-review.json" <<'REVIEW' +{ + "findings": [], + "overall_correctness": "patch is correct", + "overall_explanation": "Fixture review: the single-function addition introduces no defect at the reporting threshold.", + "overall_confidence_score": 0.9 +} +REVIEW +"$reviewer" submit --repo "$scratch" --base main --findings "$PWD/fixtures/tmp/correct-review.json" --actor open-pr-fixture > /dev/null +"$reviewer" seal --repo "$scratch" --base main > /dev/null +git -C "$scratch" add .github/reviews +git -C "$scratch" commit -qm "Seal converged self-review attestation" + +# A stub gh that records its invocation and prints a plausible URL. +mkdir -p "$PWD/fixtures/tmp/bin" +cat > "$PWD/fixtures/tmp/bin/gh" <<'STUB' +#!/bin/sh +printf '%s\n' "$@" > "$(dirname "$0")/../gh-invocation.txt" +echo "https://github.com/example/boatstack/pull/999" +STUB +chmod +x "$PWD/fixtures/tmp/bin/gh" + +touch "$PWD/fixtures/tmp/active" diff --git a/skills/open-pr/fixtures/teardown.sh b/skills/open-pr/fixtures/teardown.sh new file mode 100644 index 00000000..26996337 --- /dev/null +++ b/skills/open-pr/fixtures/teardown.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +# Remove the scratch repository and the test sentinel after success or failure. +set -eu +rm -rf "$PWD/fixtures/tmp" diff --git a/skills/open-pr/fixtures/test.json b/skills/open-pr/fixtures/test.json new file mode 100644 index 00000000..09366404 --- /dev/null +++ b/skills/open-pr/fixtures/test.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "setup": [["bash", "fixtures/setup.sh"]], + "after_response": {}, + "teardown": [["bash", "fixtures/teardown.sh"]] +} diff --git a/skills/open-pr/go.mod b/skills/open-pr/go.mod new file mode 100644 index 00000000..c11e6804 --- /dev/null +++ b/skills/open-pr/go.mod @@ -0,0 +1,10 @@ +module open-pr + +go 1.26.5 + +require github.com/operatorstack/yield v0.1.38 + +require ( + github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect + golang.org/x/text v0.14.0 // indirect +) diff --git a/skills/open-pr/go.sum b/skills/open-pr/go.sum new file mode 100644 index 00000000..b638f93e --- /dev/null +++ b/skills/open-pr/go.sum @@ -0,0 +1,8 @@ +github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= +github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= +github.com/operatorstack/yield v0.1.38 h1:QRAcWr2Do5S0wucJiDV2EXd6QP3JpgBt1YFl/rebAEY= +github.com/operatorstack/yield v0.1.38/go.mod h1:0cwOszGHYM1O8syOxJ3+NIsgR/h5JwCC680agfTKYVs= +github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 h1:KRzFb2m7YtdldCEkzs6KqmJw4nqEVZGK7IN2kJkjTuQ= +github.com/santhosh-tekuri/jsonschema/v6 v6.0.2/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU= +golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= +golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= diff --git a/skills/open-pr/main.go b/skills/open-pr/main.go new file mode 100644 index 00000000..a14c4fed --- /dev/null +++ b/skills/open-pr/main.go @@ -0,0 +1,262 @@ +// Skill workflow: open the pull request for the current branch with a +// Boatstack-structured description. +// +// Opening a pull request is a governed boundary, not a formatting step: the +// workflow refuses unless the committed review attestation verifies for the +// exact head tree (the same check CI performs), so a pull request can only +// be opened for a head whose self-review converged and was sealed. The +// description is assembled deterministically from agent-drafted sections +// plus facts the workflow gathers itself: the commit list, the attestation +// binding, and any residual (non-blocking) findings of the converged review. +package main + +import ( + "encoding/base64" + "encoding/json" + "fmt" + "strings" + + "github.com/operatorstack/yield/sdk/yield" +) + +// prelude prepares every command's execution context. Commands run with the +// skill directory as the working directory. When the test sentinel exists +// (created by fixtures/setup.sh under yskill's fixture runner), commands +// operate on a scratch repository with a local bare remote and a stub `gh`, +// so tests never push anywhere real and never open a real pull request. +// +// The reviewer is rebuilt in every command from the current tree, matching +// the self-review skills. +const prelude = `set -eu +root="$(git rev-parse --show-toplevel)" +if [ -f fixtures/tmp/active ]; then + repo="$PWD/fixtures/tmp/repo"; base=main; gh="$PWD/fixtures/tmp/bin/gh" +else + repo="$root"; base=origin/main; gh=gh +fi +tmp="${TMPDIR:-/tmp}/boatstack-open-pr" +mkdir -p "$tmp" +reviewer="$tmp/boatstack-reviewer" +go build -C "$root/boatstack" -o "$reviewer" ./cmd/boatstack-reviewer +` + +const draftSchema = `{ + "type": "object", + "required": ["title", "boundary", "transition", "evidence"], + "properties": { + "title": {"type": "string", "minLength": 8, "maxLength": 72}, + "boundary": {"type": "string", "minLength": 1}, + "transition": {"type": "string", "minLength": 1}, + "evidence": {"type": "string", "minLength": 1} + }, + "additionalProperties": false +}` + +type verifyOutput struct { + Verified bool `json:"verified"` + ReceiptPath string `json:"receipt_path"` + ProgramFingerprint string `json:"program_fingerprint"` + Failures []string `json:"failures"` +} + +type attestation struct { + ReviewedTree string `json:"reviewed_tree"` + ProgramFingerprint string `json:"program_fingerprint"` +} + +type showOutput struct { + Mode string `json:"mode"` + Review json.RawMessage `json:"review"` +} + +func main() { + yield.Main(func(ctx *yield.Context) (yield.Outcome, error) { + build := ctx.RunCommand("build-reviewer", + prelude+`true`, 600) + ctx.Require(build.ExitCode == 0, "boatstack-reviewer builds from the current tree", build) + + // Preconditions: a named branch, a clean tracked worktree, and + // commits ahead of the base. Each violation names itself. + preconditions := ctx.RunCommand("preconditions", + prelude+`branch="$(git -C "$repo" symbolic-ref --short HEAD)" +[ "$branch" != main ] && [ "$branch" != master ] || { echo "the base branch itself cannot become a pull request" >&2; exit 1; } +[ -z "$(git -C "$repo" status --porcelain --untracked-files=no)" ] || { echo "the worktree has uncommitted tracked changes; commit or stash them first" >&2; exit 1; } +ahead="$(git -C "$repo" rev-list --count "$base..HEAD")" +[ "$ahead" -gt 0 ] || { echo "the branch has no commits ahead of $base" >&2; exit 1; } +printf '%s\n' "$branch"`, 60) + if preconditions.ExitCode != 0 { + return yield.Outcome{}, ctx.Refused(strings.TrimSpace(preconditions.Stderr)) + } + branch := strings.TrimSpace(preconditions.Stdout) + + // Gate: the committed attestation must verify for the exact head + // tree — the same deterministic check CI performs. An unverified + // head has no business becoming a pull request. + verify := ctx.RunCommand("verify-attestation", + prelude+`"$reviewer" verify --repo "$repo" --dir "$repo/.github/reviews" --base "$base" --head HEAD`, 120) + if verify.ExitCode != 0 { + return yield.Outcome{}, ctx.Refused( + "the review attestation does not verify for this head; run skills/self-review-solve to converge and seal, commit the attestation, then retry: " + + strings.TrimSpace(verify.Stderr)) + } + var verified verifyOutput + if err := json.Unmarshal([]byte(verify.Stdout), &verified); err != nil { + return yield.Outcome{}, err + } + ctx.Require(verified.Verified, "the committed attestation verifies for the head tree", verified) + + receipt := ctx.RunCommand("read-attestation", + prelude+`cat `+shellQuote(verified.ReceiptPath), 30) + ctx.Require(receipt.ExitCode == 0, "the committed attestation is readable", receipt) + var attested attestation + if err := json.Unmarshal([]byte(receipt.Stdout), &attested); err != nil { + return yield.Outcome{}, err + } + + // Evidence the description is assembled from: the exact commit + // list, the change shape, and any release notes the branch adds. + commits := ctx.RunCommand("commits", + prelude+`git -C "$repo" log --reverse --format='- %s' "$base..HEAD"`, 60) + ctx.Require(commits.ExitCode == 0, "the branch commit list is readable", commits) + diffstat := ctx.RunCommand("diffstat", + prelude+`git -C "$repo" diff --stat "$base..HEAD" | tail -30`, 60) + ctx.Require(diffstat.ExitCode == 0, "the branch diffstat is readable", diffstat) + notes := ctx.RunCommand("release-notes", + prelude+`git -C "$repo" diff --name-only --diff-filter=A "$base..HEAD" -- release-notes/ || true`, 60) + + // Residuals of the converged review are part of the story the + // reviewer of the pull request should see. Best effort: a missing + // or non-converged local instance simply contributes none. + var residuals []string + if shown := ctx.RunCommand("residuals", + prelude+`"$reviewer" show --repo "$repo" --base "$base" || true`, 60); shown.ExitCode == 0 { + var out showOutput + if err := json.Unmarshal([]byte(shown.Stdout), &out); err == nil && out.Mode == "converged" { + residuals = residualTitles(out.Review) + } + } + + instruction := "Draft the pull request description sections for this branch. " + + "boundary: one short paragraph naming the durable control boundary this change crosses and why. " + + "transition: what the system refused or allowed before versus after, stated as behavior, not file names. " + + "evidence: the checks that establish the change works — suites run, regression tests added, and what each proves. " + + "title: imperative, at most 72 characters. Write for a reviewer who has not followed the work." + draft := ctx.AgentTask("draft", instruction, + map[string]any{ + "branch": branch, + "commits": strings.TrimSpace(commits.Stdout), + "diffstat": strings.TrimSpace(diffstat.Stdout), + "release_notes": strings.TrimSpace(notes.Stdout), + "attestation": attested, + "residual_findings": residuals, + }, + json.RawMessage(draftSchema)) + var sections struct { + Title string `json:"title"` + Boundary string `json:"boundary"` + Transition string `json:"transition"` + Evidence string `json:"evidence"` + } + if err := json.Unmarshal(draft, §ions); err != nil { + return yield.Outcome{}, err + } + + body := assembleBody(sections.Boundary, sections.Transition, sections.Evidence, + strings.TrimSpace(commits.Stdout), attested, residuals) + writeChunked(ctx, "title", "$tmp/pr-title.txt", []byte(sections.Title)) + writeChunked(ctx, "body", "$tmp/pr-body.md", []byte(body)) + + push := ctx.RunCommand("push", + prelude+`git -C "$repo" push -u origin `+shellQuote(branch), 300) + if push.ExitCode != 0 { + return yield.Outcome{}, ctx.Blocked("the branch did not push: " + strings.TrimSpace(push.Stderr)) + } + + create := ctx.RunCommand("create-pr", + prelude+`cd "$repo" && "$gh" pr create --title "$(cat "$tmp/pr-title.txt")" --body-file "$tmp/pr-body.md"`, 300) + if create.ExitCode != 0 { + return yield.Outcome{}, ctx.Blocked("the pull request was not created: " + strings.TrimSpace(create.Stderr)) + } + + return ctx.Complete(map[string]any{ + "pull_request": strings.TrimSpace(create.Stdout), + "title": sections.Title, + "branch": branch, + "reviewed_tree": attested.ReviewedTree, + "program_fingerprint": attested.ProgramFingerprint, + "residual_findings": residuals, + }) + }) +} + +// assembleBody renders the fixed pull-request structure: the agent owns the +// prose of each section, the workflow owns the facts. +func assembleBody(boundary, transition, evidence, commits string, attested attestation, residuals []string) string { + var b strings.Builder + b.WriteString("## Boundary\n\n" + strings.TrimSpace(boundary) + "\n\n") + b.WriteString("## Transition\n\n" + strings.TrimSpace(transition) + "\n\n") + b.WriteString("## Evidence\n\n" + strings.TrimSpace(evidence) + "\n\n") + if commits != "" { + b.WriteString("### Commits\n\n" + commits + "\n\n") + } + b.WriteString("### Self-review attestation\n\n") + b.WriteString("- reviewed tree: `" + attested.ReviewedTree + "`\n") + b.WriteString("- program fingerprint: `" + attested.ProgramFingerprint + "`\n") + b.WriteString("- the review-verified CI job re-derives both facts deterministically from the base-admitted policy\n") + if len(residuals) > 0 { + b.WriteString("\n### Residual findings (non-blocking, recorded by the converged review)\n\n") + for _, residual := range residuals { + b.WriteString("- " + residual + "\n") + } + } + return b.String() +} + +// residualTitles lists the non-blocking (P2/P3) finding titles of a review. +func residualTitles(review json.RawMessage) []string { + var parsed struct { + Findings []struct { + Title string `json:"title"` + Priority int `json:"priority"` + } `json:"findings"` + } + if err := json.Unmarshal(review, &parsed); err != nil { + return nil + } + var titles []string + for _, finding := range parsed.Findings { + if finding.Priority >= 2 { + titles = append(titles, fmt.Sprintf("P%d: %s", finding.Priority, finding.Title)) + } + } + return titles +} + +// writeChunked stages content through bounded base64 chunks so no single +// command string approaches the platform's per-argument size cap, then +// decodes it to the target path (a $tmp-relative shell expression). +func writeChunked(ctx *yield.Context, idPrefix, target string, content []byte) { + encoded := base64.StdEncoding.EncodeToString(content) + const chunkSize = 65536 + for i, part := 0, 1; i < len(encoded); i, part = i+chunkSize, part+1 { + end := i + chunkSize + if end > len(encoded) { + end = len(encoded) + } + redirect := ">>" + if i == 0 { + redirect = ">" + } + written := ctx.RunCommand(fmt.Sprintf("%s-part-%d", idPrefix, part), + prelude+`printf '%s' '`+encoded[i:end]+`' `+redirect+` "`+target+`.b64"`, 60) + ctx.Require(written.ExitCode == 0, "the content chunk is staged", written) + } + decoded := ctx.RunCommand(idPrefix+"-decode", + prelude+`base64 -d < "`+target+`.b64" > "`+target+`"`, 60) + ctx.Require(decoded.ExitCode == 0, "the staged content decodes to its file", decoded) +} + +// shellQuote single-quotes a value for safe interpolation into a command. +func shellQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" +} diff --git a/skills/open-pr/skill.json b/skills/open-pr/skill.json new file mode 100644 index 00000000..939e85ec --- /dev/null +++ b/skills/open-pr/skill.json @@ -0,0 +1 @@ +{"version":1,"language":"go","run":["go","run","-mod=readonly","."]} diff --git a/skills/self-review-solve/SKILL.md b/skills/self-review-solve/SKILL.md index 3bd0f051..6cf94a88 100644 --- a/skills/self-review-solve/SKILL.md +++ b/skills/self-review-solve/SKILL.md @@ -5,12 +5,14 @@ description: "Resolve the Boatstack self-review: fix open findings or run a fres Drive the supervisory-control self-review of the current branch to convergence. The workflow decides from the committed control state what is -needed: open findings are fixed in code and committed, an unreviewed tree -gets a fresh review, an escalated loop asks before reopening, and a -converged loop is sealed — the minimal attestation (reviewed tree + program -fingerprint) is committed locally. This skill never pushes: pushing the -branch is the user's decision, and CI verifies the attestation whenever the -push happens. +needed: open blocking findings (P0/P1) are fixed in code and committed, an +unreviewed tree gets a fresh review, an escalated loop asks before +reopening, and a converged loop is sealed — the minimal attestation +(reviewed tree + program fingerprint) is committed locally. Residual P2/P3 +findings never block convergence and are never fixed by this loop; they are +listed in the completion payload for the user to decide about. This skill +never pushes: pushing the branch is the user's decision, and CI verifies +the attestation whenever the push happens. Run from the repository root: @@ -25,7 +27,7 @@ Follow each returned operation exactly. Answer it directly: .yield/bin/yskill respond --value --skill 'skills/self-review-solve' Do not skip an operation or invent a response. When the run asks you to fix -findings, edit the code, run the relevant tests, and commit before -responding. When it asks for a review, read the prompt file it names, review +findings, fix only the blocking (P0/P1) ones, run the relevant tests, and +commit before responding; leave residual P2/P3 findings alone. When it asks for a review, read the prompt file it names, review only the committed range it names, and return only the schema-valid JSON object. diff --git a/skills/self-review-solve/main.go b/skills/self-review-solve/main.go index 9dbff369..64062e66 100644 --- a/skills/self-review-solve/main.go +++ b/skills/self-review-solve/main.go @@ -20,12 +20,19 @@ import ( // skill directory as the working directory. When the test sentinel exists // (created by fixtures/setup.sh under yskill's fixture runner), commands // operate on the scratch repository so tests never touch real review state. +// +// The reviewer is rebuilt in every command, not once per run: the Go build +// cache makes an unchanged rebuild subsecond, and a fix commit that changes +// the reviewer's own code (for example a policy or law repair demanded by +// the review itself) takes effect for the very next resolve, submit, and +// seal instead of leaving the run on a binary snapshot from run start. const prelude = `set -eu root="$(git rev-parse --show-toplevel)" if [ -f fixtures/tmp/active ]; then repo="$PWD/fixtures/tmp/repo"; base=main; else repo="$root"; base=origin/main; fi tmp="${TMPDIR:-/tmp}/boatstack-self-review-solve" mkdir -p "$tmp" reviewer="$tmp/boatstack-reviewer" +go build -C "$root/boatstack" -o "$reviewer" ./cmd/boatstack-reviewer ` const ( @@ -53,10 +60,11 @@ type statusOutput struct { WorktreeDirty bool `json:"worktree_dirty"` ReviewedTree string `json:"reviewed_tree"` Rounds []struct { - Index int `json:"index"` - Verdict string `json:"verdict"` - Measure int `json:"measure"` - ReviewedTree string `json:"reviewed_tree"` + Index int `json:"index"` + Verdict string `json:"verdict"` + Measure int `json:"measure"` + BlockingMeasure int `json:"blocking_measure"` + ReviewedTree string `json:"reviewed_tree"` } `json:"rounds"` } `json:"observation"` } @@ -72,9 +80,10 @@ type resolveOutput struct { type showOutput struct { Mode string `json:"mode"` Round struct { - Index int `json:"index"` - Verdict string `json:"verdict"` - Measure int `json:"measure"` + Index int `json:"index"` + Verdict string `json:"verdict"` + Measure int `json:"measure"` + BlockingMeasure int `json:"blocking_measure"` } `json:"round"` Review json.RawMessage `json:"review"` } @@ -100,7 +109,7 @@ func main() { } if mode == "escalated" { answer := ctx.AskUser("reopen", - "The review loop escalated (the convergence measure stalled or the round bound was reached). Reopen a fresh review generation?", + "The review loop escalated (the blocking measure stalled or the round bound was reached). Reopen a fresh review generation?", yield.Option{Value: "yes", Label: "Reopen and continue"}, yield.Option{Value: "no", Label: "Stop; a human will handle it"}) if answer != "yes" { @@ -122,8 +131,10 @@ func main() { if mode == "findings-open" && !observed.treeDrifted() { shown := show(ctx, "open-findings"+tag) fixRaw := ctx.AgentTask("fix"+tag, - "Fix every finding of this recorded review in the repository under review: edit the code, "+ - "run the relevant tests, and commit the fixes with a clear message. Do not touch .github/reviews "+ + "Fix only the BLOCKING findings (priority P0 or P1) of this recorded review in the repository under review: "+ + "edit the code, run the relevant tests, and commit the fixes with a clear message. "+ + "Leave P2/P3 findings alone — they are residuals the convergence law records but does not demand; "+ + "the user decides about them separately. Do not touch .github/reviews "+ "and do not weaken tests to make findings disappear. Report what you changed.", map[string]any{"round": shown.Round, "review": json.RawMessage(shown.Review)}, json.RawMessage(fixReportSchema)) @@ -182,10 +193,15 @@ func main() { if mode != "converged" { return yield.Outcome{}, ctx.Blocked(fmt.Sprintf( - "the review did not converge within %d attempts (measures %v); the remaining findings need a human decision", + "the review did not converge within %d attempts (blocking measures %v); the remaining blocking findings need a human decision", maxAttempts, measures)) } + // The converged round may carry residual (P2/P3) findings; they are + // surfaced for the user to decide about, never fixed by this loop. + converged := show(ctx, "converged-round") + residuals := residualTitles(converged.Review) + seal := ctx.RunCommand("seal", prelude+`"$reviewer" seal --repo "$repo" --base "$base"`, 120) ctx.Require(seal.ExitCode == 0, "the converged review seals a receipt", seal) @@ -210,14 +226,21 @@ else fi`, 60) ctx.Require(commit.ExitCode == 0, "the sealed attestation is committed with the change", commit) - return ctx.Complete(map[string]any{ - "mode": "converged", - "receipt": sealed.Sealed, - "fingerprint": sealed.Fingerprint, - "reviewed_tree": sealed.ReviewedTree, + result := map[string]any{ + "mode": "converged", + "receipt": sealed.Sealed, + "fingerprint": sealed.Fingerprint, + "reviewed_tree": sealed.ReviewedTree, + "blocking_measure": converged.Round.BlockingMeasure, "guidance": "the minimal attestation is committed locally and is NOT pushed; " + "push whenever ready — the review-verified CI job verifies it deterministically", - }) + } + if len(residuals) > 0 { + result["residual_findings"] = residuals + result["guidance"] = result["guidance"].(string) + + "; residual P2/P3 findings are listed for the user to decide — the loop does not fix them" + } + return ctx.Complete(result) }) } @@ -243,14 +266,36 @@ func writeCandidate(ctx *yield.Context, idPrefix string, review []byte) { } } +// measures projects the recorded rounds onto the blocking measure — the +// quantity the convergence law drives to zero. func (o statusOutput) measures() []int { measures := make([]int, 0, len(o.Observation.Rounds)) for _, round := range o.Observation.Rounds { - measures = append(measures, round.Measure) + measures = append(measures, round.BlockingMeasure) } return measures } +// residualTitles lists the non-blocking (P2/P3) finding titles of a review. +func residualTitles(review json.RawMessage) []string { + var parsed struct { + Findings []struct { + Title string `json:"title"` + Priority int `json:"priority"` + } `json:"findings"` + } + if err := json.Unmarshal(review, &parsed); err != nil { + return nil + } + var titles []string + for _, finding := range parsed.Findings { + if finding.Priority >= 2 { + titles = append(titles, fmt.Sprintf("P%d: %s", finding.Priority, finding.Title)) + } + } + return titles +} + // treeDrifted reports whether the current reviewed tree moved past the last // recorded round's tree — the condition under which a converged instance // needs a fresh generation before sealing. diff --git a/skills/self-review/SKILL.md b/skills/self-review/SKILL.md index ef952ce6..72b8d271 100644 --- a/skills/self-review/SKILL.md +++ b/skills/self-review/SKILL.md @@ -11,6 +11,10 @@ seals a receipt, never commits, and never pushes; when the run completes, report the verdict in the conversation and stop. Sealing and committing belong to the `self-review-solve` skill or an explicit user request. +Convergence is decided by the blocking boundary: only P0/P1 findings block. +A converged round may carry residual P2/P3 findings; the skill reports their +titles for the user to weigh — they are recorded data, not demanded work. + Run from the repository root: .yield/bin/yskill run 'skills/self-review' diff --git a/skills/self-review/main.go b/skills/self-review/main.go index ef13f39c..9e7d32e7 100644 --- a/skills/self-review/main.go +++ b/skills/self-review/main.go @@ -19,12 +19,18 @@ import ( // skill directory as the working directory. When the test sentinel exists // (created by fixtures/setup.sh under yskill's fixture runner), commands // operate on the scratch repository so tests never touch real review state. +// +// The reviewer is rebuilt in every command, not once per run: the Go build +// cache makes an unchanged rebuild subsecond, and a run whose commits change +// the reviewer's own code (or the branch under review) always drives the +// binary compiled from the current tree instead of a snapshot from run start. const prelude = `set -eu root="$(git rev-parse --show-toplevel)" if [ -f fixtures/tmp/active ]; then repo="$PWD/fixtures/tmp/repo"; base=main; else repo="$root"; base=origin/main; fi tmp="${TMPDIR:-/tmp}/boatstack-self-review" mkdir -p "$tmp" reviewer="$tmp/boatstack-reviewer" +go build -C "$root/boatstack" -o "$reviewer" ./cmd/boatstack-reviewer ` const actor = "yield-self-review" @@ -60,14 +66,37 @@ type resolveOutput struct { type showOutput struct { Mode string `json:"mode"` Round struct { - Index int `json:"index"` - Verdict string `json:"verdict"` - Measure int `json:"measure"` - FindingCount int `json:"finding_count"` + Index int `json:"index"` + Verdict string `json:"verdict"` + Measure int `json:"measure"` + BlockingMeasure int `json:"blocking_measure"` + FindingCount int `json:"finding_count"` } `json:"round"` Review json.RawMessage `json:"review"` } +// residualTitles lists the non-blocking (P2/P3) finding titles of a review. +// Blocking is P0/P1 under the admitted policy; residuals are recorded with +// the round but never drive another round. +func residualTitles(review json.RawMessage) []string { + var parsed struct { + Findings []struct { + Title string `json:"title"` + Priority int `json:"priority"` + } `json:"findings"` + } + if err := json.Unmarshal(review, &parsed); err != nil { + return nil + } + var titles []string + for _, finding := range parsed.Findings { + if finding.Priority >= 2 { + titles = append(titles, fmt.Sprintf("P%d: %s", finding.Priority, finding.Title)) + } + } + return titles +} + // writeCandidate stages the encoded candidate through bounded chunks so no // single command string approaches the platform's per-argument size cap // (MAX_ARG_STRLEN on Linux), which a large multi-finding review could @@ -120,12 +149,24 @@ func main() { if resolved.State.Mode == "converged" { rounds := resolved.Observation.Rounds if len(rounds) > 0 && rounds[len(rounds)-1].ReviewedTree == resolved.Instructions.ReviewedTree { - return ctx.Complete(map[string]any{ - "instance": resolved.Instance, - "mode": resolved.State.Mode, - "verdict": "patch is correct", - "action": reportOnlyContract, - }) + converged := ctx.RunCommand("show-converged", + prelude+`"$reviewer" show --repo "$repo" --base "$base"`, 60) + ctx.Require(converged.ExitCode == 0, "the converged round is shown from the store", converged) + var shown showOutput + if err := json.Unmarshal([]byte(converged.Stdout), &shown); err != nil { + return yield.Outcome{}, err + } + result := map[string]any{ + "instance": resolved.Instance, + "mode": resolved.State.Mode, + "verdict": shown.Round.Verdict, + "blocking_measure": shown.Round.BlockingMeasure, + "action": reportOnlyContract, + } + if residuals := residualTitles(shown.Review); len(residuals) > 0 { + result["residual_findings"] = residuals + } + return ctx.Complete(result) } // The instance converged for an older tree; new commits need a // fresh generation before a round can be recorded. @@ -190,17 +231,25 @@ func main() { }) result := map[string]any{ - "instance": resolved.Instance, - "mode": shown.Mode, - "round": shown.Round.Index, - "verdict": shown.Round.Verdict, - "measure": shown.Round.Measure, - "finding_count": shown.Round.FindingCount, - "action": reportOnlyContract, + "instance": resolved.Instance, + "mode": shown.Mode, + "round": shown.Round.Index, + "verdict": shown.Round.Verdict, + "measure": shown.Round.Measure, + "blocking_measure": shown.Round.BlockingMeasure, + "finding_count": shown.Round.FindingCount, + "action": reportOnlyContract, } - // The findings are the actionable content of an incorrect verdict; - // a correct verdict needs nothing beyond itself. - if shown.Round.Verdict != "patch is correct" { + if shown.Mode == "converged" { + // A converged round may still carry residual (P2/P3) findings; + // they are reported as titles for the user to weigh, not as + // work the loop demands. + if residuals := residualTitles(shown.Review); len(residuals) > 0 { + result["residual_findings"] = residuals + } + } else { + // Open blocking findings are the actionable content; the full + // review carries them. result["review"] = json.RawMessage(shown.Review) } return ctx.Complete(result)