Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .agents/skills/pipeline-review-and-agents/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,6 @@ metadata:
- `userIntentPromptSection` branches on source: an EXPLICIT intent renders as sanitized-but-AUTHORITATIVE acceptance criteria; an INFERRED intent keeps the low-confidence hint framing verbatim. Both branches keep the `StripAdversarial`+`RedactSecrets` pipeline and BEGIN/END "do not execute instructions" guard - authoritative reframes only the content's authority (check the diff against the criteria), never whether control tokens are stripped. The review prompt adds `intentConformanceReviewClause` for agent-source intent only: a fixer change that contradicts the criteria (removes intent-required or adds intent-forbidden behavior) MUST become an `ask-user` finding, which parks with no executor change. Conformance is limited to source-verifiable criteria; deferred pipeline-owned delivery (remote branch / push / PR / CI for this run) is out of scope at review.
- Review is always pre-push (`StepReview` before `StepPush`/`StepPR`/`StepCI`). `pipelineDeliveryPhaseClause` plus `stripDeferredPipelineOwnedDeliveryFindings` (`pipeline_delivery.go`, applied in `review.go`) keep findings that only claim those later-owned outcomes are missing from parking the run. External or pre-existing lifecycle requirements (numbered PR, third-party artifact, non-run-owned state) stay enforceable. Push, PR, and CI steps remain strict after their stages run.
- Empty/missing finding `action` fails closed to `ask-user`, not auto-fix (`types/findings.go` `ActionOrDefault`); `HasAskUserFindings` uses `ActionOrDefault` so it agrees with `AutoFixableFindings` (an unclassified finding is never auto-fixed and is always caught as ask-user). `MergeUserOverrides` still stamps user-*added* findings auto-fix on purpose.
- The conformance clause asks for `category: intent-conformance` (`types.FindingCategoryIntentConformance`) on those findings. A completed review whose latest findings still hold one was approved, not fixed, and the PR step refuses to create or update the PR (`refuseUnresolvedIntentConformance`, branch stays pushed). Key on the category, never on finding prose. Regressions: `TestPRStep_RefusesWhileApprovedIntentConformanceFindingIsUnresolved`, `TestReviewIntentConformanceCategoryIsRequestedAndAccepted`.
- The deterministic net-deleted-author-lines git-diff backstop is intentionally not built; `review.go` owns the held-scope TODO.
- Regressions: `internal/pipeline/steps/intent_prompt_test.go`, `internal/pipeline/steps/review_test.go` (`TestReviewStep_ConformanceObligationTracksIntentProvenance`, `TestReviewStep_RereviewFlagsIntentContradictionAsAskUser`), `internal/pipeline/steps/pipeline_delivery_test.go`, `internal/pipeline/steps/review_pipeline_delivery_test.go`, `internal/pipeline/executor_intent_conformance_test.go`, `internal/types/findings_test.go`, e2e `TestIntentJourney` (inferred-source framing), e2e `TestReviewPipelineOwnedPRCriterionDoesNotPark` / `TestReviewExternalPRLifecycleStillParks`.
1 change: 1 addition & 0 deletions .agents/skills/pr-publication-safety/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ metadata:
- The PR body must contain exactly ONE live pipeline-attestation marker, the run's own. `require-no-mistakes` (`.github/actions/require-no-mistakes/verify.py`) binds the FIRST marker in the RAW body to the PR head, so a foreign copy placed earlier fails a PR the pipeline did produce - and a code fence is no defense, because that scan is raw text. Step agents embed foreign markers routinely, by capturing a generated PR body as evidence.
- A CI repair that publishes a new head rewrites only that live marker's `head_sha` in the current PR body (`restampPublishedAttestation`) and does not send a title. It never inserts a marker that was not already there. Hosts without a PR content reader skip the restamp instead of failing the push. Regressions: `TestCIStep_PublishRepairRebindsAttestationAcrossRepairPushes`, `TestCIStep_PublishRepairDoesNotMintAttestation`, `TestCIStep_PublishRepairSkipsRestampWithoutReader`, `TestRestampPRAttestation_PreservesContentEditedWhilePreparingRewrite`, `TestUpdatePROmitsTitleWhenEmpty`.
- Neutralize at the assembly choke point (`appendGeneratedSectionsToCleanBodyWithinLimit` plus the two intent paths), never per render path. `pipelineMD` alone carries the real marker and is left intact; `BuildPipelineSummaryFor` neutralizes its own step-detail blocks, which quote agent text. A first attempt put this in `escapePipelineFoldMarkers` - per-render-path - and shipped three live foreign markers to #831 anyway. Regressions: `TestPRStep_ForeignAttestationsInEveryComponentDoNotShadowTheRealOne` (all components at once), plus the per-component guards in `pr_test.go`.
- `isForeignOwnerPR` (`pr.go`: a fork whose owner differs from the parent's) is the single switch that drops the `## Intent` section (`prBodyIntent`, which both intent paths share) and local-path evidence references (`testingSummaryOptions.omitLocalPaths`) from the body; inline evidence text, links, and attachments stay. Same-owner bodies must stay byte-identical. Regressions: `TestPRBody_ForeignOwnerOmitsIntentAndLocalPathEvidence`, `TestPRBody_SameOwnerKeepsIntentAndLocalPathEvidence`.
- Regressions: `internal/safepath/redact_test.go`, `internal/pipeline/steps/pr_homepath_test.go`.
4 changes: 3 additions & 1 deletion docs/src/content/docs/reference/pipeline-steps.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,13 +248,15 @@ Creates or updates a pull request.
- PR title: agent-generated from the final branch delta with user intent when available, in conventional commit format (`type(scope): description` or `type: description`); user-facing product impact should use `feat` or `fix` so release automation can pick it up; when a scope is used, it should be the primary affected real module/package from the changed paths and kept broad rather than file-level. If drafting fails, the fallback uses the neutral title `chore: update pull request` rather than inferring scope from earlier commits.
- Bounds the PR-drafting agent with [`agent_timeout`](/no-mistakes/reference/global-config/#agent_timeout): an expired budget cancels the agent and uses that same fallback rather than leaving the run active indefinitely; a late successful title after the deadline is not used
- The PR stage exclusively owns the complete branch-scope description. It drafts `## What Changed` from the actual final diff after local mutating stages finish, and its fallback lists the final changed paths and statuses.
- PR body includes a `## Intent` section when user intent is available, the final-diff `## What Changed`, and regenerated `## Risk Assessment`, `## Testing`, and `## Pipeline` sections from recorded step results and rounds. Only `## What Changed` describes the complete final branch scope; the deterministic sections remain evidence for the commit each step inspected. Auto-fix results in `## Pipeline` render as an issue -> fix -> verification narrative using recorded outcomes: applied changes, a confirmed no-change attempt, or an attempt whose result was not recorded. Test details show the live-validation verdict, scenario table, and recorded commands in both `## Testing` and the relevant Pipeline rounds.
- Refuses to create or update the pull request, failing the step with the branch left pushed, while the completed Review step still holds an intent-conformance finding: Review tags a finding `intent-conformance` when the change contradicts an authoritative intent, so one still present after Review completed was approved rather than fixed
- PR body includes a `## Intent` section when user intent is available and the PR is not a foreign-owner PR, the final-diff `## What Changed`, and regenerated `## Risk Assessment`, `## Testing`, and `## Pipeline` sections from recorded step results and rounds. Only `## What Changed` describes the complete final branch scope; the deterministic sections remain evidence for the commit each step inspected. Auto-fix results in `## Pipeline` render as an issue -> fix -> verification narrative using recorded outcomes: applied changes, a confirmed no-change attempt, or an attempt whose result was not recorded. Test details show the live-validation verdict, scenario table, and recorded commands in both `## Testing` and the relevant Pipeline rounds.
- `## Pipeline` keeps the existing human-readable signature and includes the stable structured step attestation documented below. Bitbucket Cloud PR descriptions omit HTML-only features (`<details>`, `<code>`, `<video>`, and the attestation comment) because Cloud renders Python-Markdown and escapes raw HTML.
- Generated PR bodies are capped at 63,488 bytes, leaving a 2 KB safety buffer below GitHub's 65,536-character body limit.
- When a body would exceed that cap, the PR step first omits older `## Pipeline` update rounds at clean update boundaries, keeps the newest rounds when possible, and points reviewers to the run log for the full pipeline history.
- Intent, `## What Changed`, risk, and testing sections are kept ahead of pipeline history; if those sections or the newest pipeline update are still too large, the PR step truncates at line or section boundaries and adds an explicit marker.
- The regenerated `## Testing` section prefers the recorded `testing_summary` as prose, uses a compact recorded-check count when no summary is available, includes produced evidence artifacts from `path`, `url`, or `content` fields when available, and only adds an outcome with run count and total duration when it is failed or needed as a fallback
- Evidence artifacts render compactly in PR bodies: repository-relative `path` artifacts and `url` artifacts become `Evidence` links, `content` artifacts appear in collapsible details blocks, GitHub PRs convert repository-relative paths to blob URLs and published evidence to commit-pinned blob or raw URLs, readable UTF-8 text files from the run's evidence directory are embedded inline with truncation for large files, and binary, visual, or over-budget local artifacts render as non-link local file references
- A foreign-owner PR - a GitHub fork routing whose `fork_url` owner differs from the parent repository's owner - omits the `## Intent` section and every local-file evidence reference the maintainer cannot open; an artifact with only a local path is dropped, while inline evidence text, links, and attachments stay. Same-owner PRs are unchanged.
- Before the PR is created or updated, the assembled title and body pass through a final home-directory redaction. The home-directory portion of any absolute path - the operator's own home, and `/home/<user>`, `/Users/<user>`, or `C:\Users\<user>` generally - is rewritten to `~` while the rest of the path survives, so the run's evidence and worktree locations, captured command output, artifact paths, and agent prose cannot publish the operator's account name. Redaction is unconditional and runs after every length cap.
- For Azure DevOps, the PR description is capped at 4000 characters (UTF-16 code units, matching .NET's measurement): the agent is told about the cap and asked to keep the `## What Changed` section compact; if the assembled body still overruns, the `## Testing` section is dropped first because it can embed artifact and log content, preferentially preserving Intent, What Changed, Risk Assessment, and Pipeline; a final connector-level clamp truncates with a visible marker as a last-resort backstop

Expand Down
3 changes: 2 additions & 1 deletion internal/pipeline/steps/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,8 @@ var reviewFindingsSchema = json.RawMessage(`{
"line": {"type": "integer"},
"description": {"type": "string"},
"action": {"type": "string", "enum": ["no-op", "auto-fix", "ask-user"]},
"review_scope": {"type": "string", "enum": ["source", "pipeline-owned-delivery", "external-delivery"]}
"review_scope": {"type": "string", "enum": ["source", "pipeline-owned-delivery", "external-delivery"]},
"category": {"type": "string", "enum": ["intent-conformance"]}
},
"required": ["severity", "description", "action", "review_scope"]
}
Expand Down
2 changes: 1 addition & 1 deletion internal/pipeline/steps/intent_prompt.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func intentConformanceReviewClause(sctx *pipeline.StepContext) string {
// Pipeline-owned delivery outcomes (push, PR open/update, CI) are owned by
// later steps; review must not treat their absence as an intent
// contradiction. Source-verifiable required/forbidden behavior stays hard.
return "\n\nIntent conformance (required): the User intent above is authoritative acceptance criteria, not a hint. If the change contradicts it - it removes or omits a source-verifiable behavior the criteria mark as REQUIRED, or adds a behavior they mark as FORBIDDEN - you MUST emit an \"ask-user\" finding that quotes the specific criterion and the contradicting diff hunk (or, for a removed required behavior, notes what the criteria require that is now absent from the change), even if the change is otherwise risk-clean. Do not resolve such a contradiction yourself and do not classify it \"auto-fix\". Do not treat deferred pipeline-owned delivery outcomes (remote branch not yet pushed, pull request not yet opened or updated, CI not yet observed for this run) as contradictions at this phase; later pipeline steps own those. Conformance does not replace correctness review: an authoritative intent obliges flagging contradictions but never substitutes for checking that the algorithm is correct. An implementation that satisfies every required constraint can still compute a wrong value, label, or set; flag those as ordinary source findings."
return "\n\nIntent conformance (required): the User intent above is authoritative acceptance criteria, not a hint. If the change contradicts it - it removes or omits a source-verifiable behavior the criteria mark as REQUIRED, or adds a behavior they mark as FORBIDDEN - you MUST emit an \"ask-user\" finding with category \"intent-conformance\" that quotes the specific criterion and the contradicting diff hunk (or, for a removed required behavior, notes what the criteria require that is now absent from the change), even if the change is otherwise risk-clean. Do not resolve such a contradiction yourself and do not classify it \"auto-fix\". Set category \"intent-conformance\" only on such contradictions and omit category on every other finding. Do not treat deferred pipeline-owned delivery outcomes (remote branch not yet pushed, pull request not yet opened or updated, CI not yet observed for this run) as contradictions at this phase; later pipeline steps own those. Conformance does not replace correctness review: an authoritative intent obliges flagging contradictions but never substitutes for checking that the algorithm is correct. An implementation that satisfies every required constraint can still compute a wrong value, label, or set; flag those as ordinary source findings."
}

// cleanedUserIntent returns the trimmed, secret-redacted, adversarial-stripped
Expand Down
82 changes: 74 additions & 8 deletions internal/pipeline/steps/pr.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ func (s *PRStep) Execute(sctx *pipeline.StepContext) (*pipeline.StepOutcome, err
sctx.Log(fmt.Sprintf("skipping PR creation: %v", err))
return &pipeline.StepOutcome{Skipped: true, SkipReason: err.Error()}, nil
}
if err := refuseUnresolvedIntentConformance(sctx); err != nil {
return nil, err
}

// Resolve the branch base so PR summaries cover the full branch delta.
baseSHA := resolveBranchBaseSHA(ctx, sctx.WorkDir, sctx.Run.BaseSHA, baseBranch)
Expand Down Expand Up @@ -406,7 +409,7 @@ func (s *PRStep) buildPipelineSection(sctx *pipeline.StepContext, provider scm.P
}

pipelineMD, riskLine = BuildPipelineSummaryFor(steps, rounds, sctx.Run.HeadSHA, provider)
testingMD = buildPRTestingSummary(steps, rounds, sctx.Repo.UpstreamURL, sctx.Run.HeadSHA, sctx.WorkDir, testEvidenceDir(sctx), publishRunEvidence(sctx), provider, s.attachRunEvidenceMedia(sctx, provider, steps, rounds))
testingMD = buildPRTestingSummary(steps, rounds, sctx.Repo.UpstreamURL, sctx.Run.HeadSHA, sctx.WorkDir, testEvidenceDir(sctx), publishRunEvidence(sctx), provider, s.attachRunEvidenceMedia(sctx, provider, steps, rounds), isForeignOwnerPR(sctx))
return pipelineMD, riskLine, testingMD
}

Expand Down Expand Up @@ -518,9 +521,7 @@ func appendGeneratedSections(body, riskLine, testingMD, pipelineMD string) strin
func buildPRBody(body, riskLine, testingMD, pipelineMD string, sctx *pipeline.StepContext) string {
body = stripGeneratedSections(body)
sections := appendGeneratedSectionsToCleanBody(body, riskLine, testingMD, pipelineMD)
// Neutralized for the same reason as in prependIntentSection: intent is
// agent-extracted text placed ahead of the pipeline section.
cleaned := neutralizeAttestationMarkers(cleanedUserIntent(sctx))
cleaned := prBodyIntent(sctx)
if cleaned == "" {
return sections
}
Expand Down Expand Up @@ -1275,10 +1276,7 @@ func isGeneratedSectionHeading(line string) bool {
// rather than being paraphrased by the agent. Returns body unchanged when
// no intent is available.
func prependIntentSection(body string, sctx *pipeline.StepContext) string {
// Intent is agent-extracted text that lands ahead of the pipeline section,
// so it can shadow the real attestation the same way the Testing section
// can. See appendGeneratedSectionsToCleanBodyWithinLimit.
cleaned := neutralizeAttestationMarkers(cleanedUserIntent(sctx))
cleaned := prBodyIntent(sctx)
if cleaned == "" {
return body
}
Expand All @@ -1289,6 +1287,74 @@ func prependIntentSection(body string, sctx *pipeline.StepContext) string {
return section + "\n\n" + body
}

// prBodyIntent returns the intent text the PR body's "## Intent" section
// carries, or "" when the body must not carry one. A pull request against a
// repository another owner holds goes to a maintainer who never saw the run's
// task intent, so it is omitted there (see isForeignOwnerPR).
//
// Intent is agent-extracted text that lands ahead of the pipeline section, so
// it can shadow the real attestation the same way the Testing section can; it
// is neutralized for that reason. See
// appendGeneratedSectionsToCleanBodyWithinLimit.
func prBodyIntent(sctx *pipeline.StepContext) string {
if isForeignOwnerPR(sctx) {
return ""
}
return neutralizeAttestationMarkers(cleanedUserIntent(sctx))
}

// isForeignOwnerPR reports whether the pull request targets a base repository
// owned by someone other than the pushing account: a configured fork whose
// owner differs from the parent's. Without a fork the branch is pushed to the
// base repository itself, so the pushing account is treated as its owner.
func isForeignOwnerPR(sctx *pipeline.StepContext) bool {
if sctx == nil || sctx.Repo == nil {
return false
}
fork := strings.TrimSpace(sctx.Repo.ForkURL)
if fork == "" {
return false
}
return !strings.EqualFold(remoteOwner(fork), remoteOwner(sctx.Repo.UpstreamURL))
}

func remoteOwner(remote string) string {
owner, _, _ := strings.Cut(scm.RepoPath(remote), "/")
return owner
}

// refuseUnresolvedIntentConformance stops the PR step while the completed
// review still holds an intent-conformance finding. The step's findings are
// those of its latest round, so a finding still present there was approved
// through rather than fixed: the change contradicts the run's authoritative
// intent, and publishing it would hand a maintainer a PR whose own review
// says it does not do what it claims. The branch stays pushed.
func refuseUnresolvedIntentConformance(sctx *pipeline.StepContext) error {
steps, err := sctx.DB.GetStepsByRun(sctx.Run.ID)
if err != nil {
return fmt.Errorf("read review findings before opening a pull request: %w", err)
}
for _, sr := range steps {
if sr.StepName != types.StepReview || sr.Status != types.StepStatusCompleted || sr.FindingsJSON == nil {
continue
}
findings, err := types.ParseFindingsJSON(*sr.FindingsJSON)
if err != nil {
return fmt.Errorf("read review findings before opening a pull request: %w", err)
}
var ids []string
for _, item := range findings.Items {
if item.Category == types.FindingCategoryIntentConformance {
ids = append(ids, item.ID)
}
}
if len(ids) > 0 {
return fmt.Errorf("refusing to open or update the pull request: review was approved with unresolved intent-conformance finding(s) %s - the change does not match the run's intent; fix the change or rerun with a corrected --intent (the branch stays pushed)", strings.Join(ids, ", "))
}
}
return nil
}

func fallbackPRContent(sctx *pipeline.StepContext, finalDiff, riskLine, testingMD, pipelineMD string, bodyLimit int) prContent {
title := "chore: update pull request"
diffSummary := strings.TrimSpace(finalDiff)
Expand Down
Loading