diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 00000000..f1f8ecda --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,77 @@ +# jcode Cloud domain context + +This glossary records the product language shared by the control plane, Runner, +Console, and design documents. It is intentionally small: implementation detail +belongs next to the implementation and accepted decisions belong in +`docs/02-decision-log.md`. + +## Core terms + +- **Work Item** — durable team work. A jtype Card remains the truth for Kanban + work; a provider Pull Request remains the truth for review work. Cloud does not + create a second issue system. +- **Trigger** — the fact that requests execution: Manual, JType transition, SCM + event/comment, Cron occurrence, or scoped API call. Trigger implementations + differ, but all must produce the same bounded trigger facts before dispatch. +- **Workflow Definition** — an editable, versioned authoring artifact that binds + a trigger shape, Agent Profile, requirements, timeout, typed outputs, and prompt. + Ship-R1 has code-owned built-in definitions; repository/UI authoring arrives in + Ship-R2. A definition is compiled into a Run-specific Workflow Contract. +- **Agent Profile** — a versioned role contract describing how an agent should + behave and which capabilities it requires. Built-in roles are Developer, + Reviewer, Product Manager, and Architect; a Custom profile may refine the + instructions and requirements. A profile is not a human identity and does not + grant authorization. +- **LLM Selection** — the resolved provider/model/effort projection frozen for a + Run. Ship-R1 does not introduce a separate LLM Profile resource; that remains + distinct from Agent Profile and Runner Profile even when selected from current + Project/model settings. +- **Workflow Contract** — the immutable, schema-versioned execution contract + compiled for one Run from a Workflow Definition revision, bounded trigger + facts, an Agent Profile revision, Service policy, LLM Selection, typed delivery + outputs, and verification rules. Editing a profile or Automation never mutates + an existing Run's contract. +- **Run Readiness** — a server-side evaluation of a proposed Workflow Contract + against model, repository, Plugin, Runner, persistence, and delivery + capabilities. Ship-R1 keeps existing dispatch prerequisites and records the + resolved contract; Ship-R2 exposes the shared preview/create evaluator. A + failed check never silently selects another profile or output. +- **Run** — one execution truth in Cloud: lifecycle, transcript, artifacts, + result, delivery, usage, and frozen Workflow Contract. +- **Run SCM Grant** — the immutable repository and provider authorization facts + for a Run: installation/grant revision, provider configuration revision, + repository identity, clone route, default branch, and acting provider identity. + Clone, push, Pull Request, and review writeback consume the same grant. +- **Review Revision Pair** — the provider-verified base/head commit SHAs frozen on + a Review Run. The Review Plan may additionally record the computed merge-base; + revision facts are not authorization and therefore are not part of SCM Grant. +- **Runner Profile** — an operator-owned manifest for a Runner image digest and + its executable/tool/network/persistence capabilities. It is infrastructure, + not an Agent persona. +- **Review Plan** — a deterministic plan pinned to base/head commits, with a + changed-hunk index, bounded review units, rules revision, and input coverage + ledger. Ship-R1 coverage proves what entered the reviewer, not what the model + semantically considered. +- **Typed Output** — an allowlisted intent produced by a Run and externalized by + a control-plane adapter, such as `provider_review` or `create_pull_request`. + Agent tool permission never grants an output absent from the contract. +- **Delivery** — the control-plane-owned publication of a Run result. Pull + Request delivery is lifecycle-aware by default: a one-shot success is Ready; + a long session may use Draft only as an intermediate state. Cloud never + approves or merges automatically. +- **Workspace Checkpoint** — an append-only reference to a stable Run workspace + state. A PVC is a cache and resume substrate, not the checkpoint truth. + +## Invariants + +1. Trigger identity, accountable identity, Agent Profile, Runner Profile, and + provider Bot identity are distinct concepts. +2. Display-only provenance never participates in authorization. +3. A Run executes one frozen Workflow Contract; retries explicitly choose + whether to reuse it or resolve a new revision. +4. Provider credentials never enter the Runner. A Run only receives scoped + source and credential adapters from the control plane. +5. Different trigger implementations may collect different facts, but they may + not bypass platform dispatch prerequisites or invent private dispatch paths. +6. Multi-agent execution is opt-in orchestration over isolated review/work + units. A role profile does not imply multi-agent execution. diff --git a/console/src/api/types.ts b/console/src/api/types.ts index 67e379c6..837f4b1b 100644 --- a/console/src/api/types.ts +++ b/console/src/api/types.ts @@ -650,6 +650,13 @@ export interface Run { review_output?: string; /** When the review comment was posted to the PR (idempotency marker). */ review_posted_at?: string | null; + /** Immutable built-in Workflow + Agent Profile projection resolved at Run creation. */ + execution_contract?: WorkflowContract; + /** Server-accepted review input coverage. Private changed-line anchors are never serialized. */ + review_plan?: ReviewPlan; + /** Exact provider revisions frozen before a review Run is queued. */ + pr_head_sha?: string; + pr_base_sha?: string; /** * How the run was triggered (M7 / blueprint §8): the API/console (default, * absent) or a Gitea PR comment `@jcode …` webhook. A webhook run carries the @@ -694,6 +701,65 @@ export interface Run { provenance?: RunProvenance; } +export interface WorkflowContract { + schema_version: number; + hash: string; + workflow: { id: string; name: string; revision: number; source: string; definition_hash: string }; + profile: { id: string; name: string; role: string; revision: number; instructions: string }; + trigger: { + kind: string; + origin?: string; + repository?: string; + object?: string; + action?: string; + ref?: string; + automation_id?: string; + idempotency_key: string; + concurrency_group: string; + }; + execution: { + run_kind: RunKind; + llm_selection: { model_id?: string; model_name: string; effort?: string; source: string }; + session: boolean; + permission_mode: string; + workspace_access: 'read_only' | 'read_write' | string; + provider_credentials: 'none' | string; + base_ref?: string; + timeout_seconds: number; + timeout_source: string; + }; + delivery: { outputs: Array<{ type: string; target?: string; ready_policy?: string }>; merge: 'never' | string }; + verification: { mode: string; rules_revision?: string; max_findings?: number; minimum_confidence?: number; required_records?: string[] }; + requirements: string[]; + resolved_at: string; +} + +export interface ReviewPlanFile { + path: string; + status: 'indexed' | 'skipped' | string; + reason?: string; + hunks: number; + changed_lines: number; +} + +export interface ReviewPlan { + schema_version: number; + plan_hash: string; + base_sha: string; + head_sha: string; + merge_base_sha: string; + rules_revision: string; + coverage: 'complete' | 'partial' | string; + changed_files: number; + eligible_files: number; + indexed_files: number; + changed_hunks: number; + indexed_hunks: number; + changed_lines: number; + files: ReviewPlanFile[]; + created_at: string; +} + export interface ProvenanceActorRef { kind: 'cloud_user' | 'external_actor' | 'service_principal' | 'automation_principal' | 'provider_bot' | string; id?: string; diff --git a/console/src/i18n/locales/en.ts b/console/src/i18n/locales/en.ts index f5ed4ecc..361148a2 100644 --- a/console/src/i18n/locales/en.ts +++ b/console/src/i18n/locales/en.ts @@ -2129,6 +2129,37 @@ export default { resumedFrom: 'Resumed from', resumedFromLink: 'resumed from {id}', }, + contract: { + title: 'Execution contract', + legacyUnavailable: 'This historical Run predates immutable execution contracts.', + workflow: 'Workflow', + profile: 'Agent profile', + trigger: 'Trigger', + model: 'Model', + timeout: 'Timeout', + delivery: 'Delivery', + verification: 'Verification', + technical: 'Technical contract', + hash: 'Contract hash', + definition: 'Definition', + access: 'Workspace', + requirements: 'Requirements', + }, + coverage: { + eyebrow: 'Review input', + title: 'Review coverage', + unavailable: 'Unavailable', + legacyUnavailable: 'Coverage was not recorded for this historical review Run.', + complete: 'Complete', + partial: 'Partial', + files: 'files indexed', + hunks: 'hunks indexed', + lines: 'changed lines', + baseHead: 'Base → head', + mergeBase: 'Merge base', + skipped: '{count} skipped files', + unsupported: 'unsupported', + }, origin: { fromPrComment: 'from PR comment', scheduled: 'scheduled', diff --git a/console/src/i18n/locales/zh-Hans.ts b/console/src/i18n/locales/zh-Hans.ts index 34c2b7df..ca7e695a 100644 --- a/console/src/i18n/locales/zh-Hans.ts +++ b/console/src/i18n/locales/zh-Hans.ts @@ -1963,6 +1963,37 @@ export default { resumedFrom: '恢复自', resumedFromLink: '恢复自 {id}', }, + contract: { + title: '执行契约', + legacyUnavailable: '此历史运行早于不可变执行契约功能。', + workflow: '工作流', + profile: 'Agent 配置', + trigger: '触发器', + model: '模型', + timeout: '超时', + delivery: '交付', + verification: '校验', + technical: '技术契约', + hash: '契约哈希', + definition: '定义', + access: '工作区', + requirements: '能力要求', + }, + coverage: { + eyebrow: '评审输入', + title: '评审覆盖范围', + unavailable: '不可用', + legacyUnavailable: '此历史评审运行未记录覆盖范围。', + complete: '完整', + partial: '部分', + files: '已索引文件', + hunks: '已索引代码块', + lines: '变更行', + baseHead: '基线 → 提交', + mergeBase: '共同祖先', + skipped: '跳过 {count} 个文件', + unsupported: '不支持', + }, origin: { fromPrComment: '来自 PR 评论', scheduled: '定时', diff --git a/console/src/pages/RunDetailPage.module.css b/console/src/pages/RunDetailPage.module.css index f33cf839..e2d8976e 100644 --- a/console/src/pages/RunDetailPage.module.css +++ b/console/src/pages/RunDetailPage.module.css @@ -154,6 +154,30 @@ .empty { margin: 0; padding: var(--space-8); border: 1px solid var(--color-border); border-radius: var(--radius-lg); color: var(--color-text-faint); font-size: var(--fs-sm); text-align: center; } .reviewOutput { padding: var(--space-5); border: 1px solid var(--color-border); border-radius: var(--radius-xl); } .reviewProgress { display: grid; gap: var(--space-5); } +.reviewCoverage { display: grid; gap: var(--space-4); padding: var(--space-5); border: 1px solid var(--color-border); border-radius: var(--radius-xl); background: var(--color-bg-inset); } +.reviewCoverage header { display: flex; align-items: flex-start; justify-content: space-between; gap: var(--space-4); } +.reviewCoverage header > div { display: grid; gap: 3px; } +.reviewCoverage header span { color: var(--color-text-faint); font-family: var(--font-mono); font-size: 10px; font-weight: var(--fw-semibold); letter-spacing: var(--tracking-label); text-transform: uppercase; } +.reviewCoverage h2 { margin: 0; color: var(--color-text); font-size: var(--fs-sm); } +.reviewCoverage header strong { padding: 3px var(--space-2); border: 1px solid var(--color-border); border-radius: var(--radius-pill); color: var(--color-text-muted); font-family: var(--font-mono); font-size: 10px; text-transform: uppercase; } +.reviewCoverage header strong[data-coverage='complete'] { border-color: color-mix(in srgb, var(--status-succeeded-fg) 35%, var(--color-border)); color: var(--status-succeeded-fg); } +.reviewCoverage header strong[data-coverage='partial'] { border-color: color-mix(in srgb, var(--status-scheduling-fg) 35%, var(--color-border)); color: var(--status-scheduling-fg); } +.reviewCoverage > p { margin: 0; color: var(--color-text-faint); font-size: var(--fs-caption); } +.coverageMetrics { display: grid; grid-template-columns: repeat(3, minmax(0, 1fr)); border: 1px solid var(--color-border); border-radius: var(--radius-lg); overflow: hidden; } +.coverageMetrics > div { display: grid; gap: 2px; padding: var(--space-3); } +.coverageMetrics > div + div { border-left: 1px solid var(--color-border); } +.coverageMetrics b { color: var(--color-text); font-family: var(--font-mono); font-size: var(--fs-sm); font-weight: var(--fw-medium); } +.coverageMetrics span { color: var(--color-text-faint); font-size: 10px; } +.coverageRevisions { display: grid; gap: var(--space-2); margin: 0; } +.coverageRevisions > div { display: grid; grid-template-columns: 92px minmax(0, 1fr); gap: var(--space-3); } +.coverageRevisions dt { color: var(--color-text-faint); font-size: var(--fs-tiny); } +.coverageRevisions dd { margin: 0; color: var(--color-text-muted); font-size: var(--fs-tiny); overflow-wrap: anywhere; } +.coverageSkipped { color: var(--color-text-faint); font-size: var(--fs-tiny); } +.coverageSkipped summary { width: max-content; cursor: pointer; } +.coverageSkipped ul { display: grid; gap: var(--space-2); margin: var(--space-3) 0 0; padding: 0; list-style: none; } +.coverageSkipped li { display: flex; min-width: 0; justify-content: space-between; gap: var(--space-3); } +.coverageSkipped code { overflow: hidden; color: var(--color-text-muted); text-overflow: ellipsis; white-space: nowrap; } +.coverageSkipped li span { flex: none; } @media (max-width: 940px) { .taskLayout { grid-template-columns: minmax(0, 1fr); align-content: start; overflow-y: auto; } diff --git a/console/src/pages/RunDetailPage.test.tsx b/console/src/pages/RunDetailPage.test.tsx index 76872e57..039a500f 100644 --- a/console/src/pages/RunDetailPage.test.tsx +++ b/console/src/pages/RunDetailPage.test.tsx @@ -112,6 +112,67 @@ function renderPage(client: ApiClient, seed?: Run) { } describe('RunDetailPage — resilient error states', () => { + it('shows the frozen workflow contract and deterministic review coverage', async () => { + const { client, ctl } = makeClient(); + const run = baseRun({ + kind: 'review', + status: 'succeeded', + finished_at: '2026-07-07T00:05:00Z', + review_output: 'No findings.', + execution_contract: { + schema_version: 1, + hash: 'sha256:contract', + workflow: { id: 'builtin:pull-request-review', name: 'Pull Request Review', revision: 1, source: 'builtin', definition_hash: 'sha256:definition' }, + profile: { id: 'builtin:reviewer', name: 'Reviewer', role: 'reviewer', revision: 1, instructions: 'Review exact revisions.' }, + trigger: { kind: 'scm', origin: 'automation', idempotency_key: 'delivery-1', concurrency_group: 'svc-1:pull_request:42' }, + execution: { + run_kind: 'review', + llm_selection: { model_name: 'glm-5.2', source: 'resolved_run' }, + session: false, + permission_mode: 'full_access', + workspace_access: 'read_only', + provider_credentials: 'none', + timeout_seconds: 1800, + timeout_source: 'project_override', + }, + delivery: { outputs: [{ type: 'provider_review', target: 'trigger_pr' }], merge: 'never' }, + verification: { mode: 'structured_review', rules_revision: 'review-v2', max_findings: 50, minimum_confidence: 80 }, + requirements: ['source.read', 'git', 'ripgrep', 'scm.review.write'], + resolved_at: '2026-07-07T00:00:00Z', + }, + review_plan: { + schema_version: 1, + plan_hash: 'sha256:plan', + base_sha: '1111111111111111111111111111111111111111', + head_sha: '2222222222222222222222222222222222222222', + merge_base_sha: '1111111111111111111111111111111111111111', + rules_revision: 'review-v2', + coverage: 'partial', + changed_files: 2, + eligible_files: 1, + indexed_files: 1, + changed_hunks: 3, + indexed_hunks: 2, + changed_lines: 17, + files: [ + { path: 'src/main.ts', status: 'indexed', hunks: 2, changed_lines: 17 }, + { path: 'assets/logo.png', status: 'skipped', reason: 'binary', hunks: 1, changed_lines: 0 }, + ], + created_at: '2026-07-07T00:00:10Z', + }, + }); + ctl.getRun.mockResolvedValue(run); + renderPage(client, run); + + expect(await screen.findByTestId('workflow-contract')).toBeTruthy(); + expect(screen.getByText('Pull Request Review v1')).toBeTruthy(); + expect(screen.getByText('30m · project_override')).toBeTruthy(); + const coverage = screen.getByTestId('review-coverage'); + expect(coverage.querySelector('[data-coverage="partial"]')).toBeTruthy(); + expect(screen.getByText('1/2')).toBeTruthy(); + expect(screen.getByText('17')).toBeTruthy(); + }); + it('shows captured Run usage in the inspector without merging cost sources', async () => { const { client, ctl } = makeClient(); const value = baseRun({ diff --git a/console/src/pages/RunDetailPage.tsx b/console/src/pages/RunDetailPage.tsx index d2e1ad26..98fc7e30 100644 --- a/console/src/pages/RunDetailPage.tsx +++ b/console/src/pages/RunDetailPage.tsx @@ -19,7 +19,7 @@ import { } from '../api/queries'; import { useApi } from '../api/ApiProvider'; import { ApiError } from '../api/client'; -import { isTerminal, type FailureReason, type ProjectModel, type ProvenanceActorRef, type ResumeSessionOptions, type Run, type RunProvenance } from '../api/types'; +import { isTerminal, type FailureReason, type ProjectModel, type ProvenanceActorRef, type ResumeSessionOptions, type ReviewPlan, type Run, type RunProvenance, type WorkflowContract } from '../api/types'; import { Button } from '../components/Button'; import { DiffView } from '../components/DiffView'; import { Markdown } from '../components/Markdown'; @@ -369,6 +369,8 @@ export function RunDetailPage() { /> + {isReview && } + {isReview && current.review_output ? (
) : isReview && !terminalRun ? ( @@ -800,6 +802,7 @@ function RunInspector({
+
{run.permission_mode === 'approval' ? t('runDetail.inspector.askBeforeActions') : t('runDetail.permission.fullAccess')} @@ -831,6 +834,91 @@ function RunInspector({ ); } +function WorkflowContractSection({ contract }: { contract?: WorkflowContract }) { + const { t } = useTranslation(); + if (!contract) { + return ( + +

{t('runDetail.contract.legacyUnavailable')}

+ + ); + } + const output = contract.delivery.outputs[0]; + const delivery = output + ? [output.type, output.target, output.ready_policy].filter(Boolean).join(' · ') + : t('runDetail.inspector.unavailable'); + const verification = [contract.verification.mode, contract.verification.rules_revision].filter(Boolean).join(' · '); + return ( + +
+ {contract.workflow.name} v{contract.workflow.revision} + {contract.profile.name} · {contract.profile.role} + {contract.trigger.kind} + {contract.execution.llm_selection.model_name || t('runDetail.inspector.unavailable')} + {formatTimeout(contract.execution.timeout_seconds)} · {contract.execution.timeout_source} + {delivery} + {verification} +
+
+ {t('runDetail.contract.technical')} +
+ {contract.hash} + {contract.workflow.definition_hash} + {contract.execution.workspace_access} + {contract.requirements.join(', ')} +
+
+ + ); +} + +function ReviewCoverageCard({ plan }: { plan?: ReviewPlan }) { + const { t } = useTranslation(); + if (!plan) { + return ( +
+
{t('runDetail.coverage.eyebrow')}

{t('runDetail.coverage.title')}

{t('runDetail.coverage.unavailable')}
+

{t('runDetail.coverage.legacyUnavailable')}

+
+ ); + } + const skipped = plan.files.filter((file) => file.status !== 'indexed'); + const coverageLabel = plan.coverage === 'complete' ? t('runDetail.coverage.complete') : t('runDetail.coverage.partial'); + return ( +
+
+
{t('runDetail.coverage.eyebrow')}

{t('runDetail.coverage.title')}

+ {coverageLabel} +
+
+
{plan.indexed_files}/{plan.changed_files}{t('runDetail.coverage.files')}
+
{plan.indexed_hunks}/{plan.changed_hunks}{t('runDetail.coverage.hunks')}
+
{plan.changed_lines}{t('runDetail.coverage.lines')}
+
+
+
{t('runDetail.coverage.baseHead')}
{shortRevision(plan.base_sha)} → {shortRevision(plan.head_sha)}
+
{t('runDetail.coverage.mergeBase')}
{shortRevision(plan.merge_base_sha)}
+
+ {skipped.length > 0 && ( +
+ {t('runDetail.coverage.skipped', { count: skipped.length })} +
    {skipped.map((file) =>
  • {file.path}{file.reason || t('runDetail.coverage.unsupported')}
    • )}
+
+ )} +
+ ); +} + +function formatTimeout(seconds: number): string { + if (seconds % 3600 === 0) return `${seconds / 3600}h`; + if (seconds % 60 === 0) return `${seconds / 60}m`; + return `${seconds}s`; +} + +function shortRevision(value: string): string { + return value.length > 10 ? value.slice(0, 10) : value; +} + function ProvenanceSection({ provenance }: { provenance?: RunProvenance }) { const { t } = useTranslation(); const executedFor = provenance?.executed_for; diff --git a/design/README.md b/design/README.md index 495498fd..e46a1089 100644 --- a/design/README.md +++ b/design/README.md @@ -99,3 +99,7 @@ The designs deliberately stay within current backend contracts: setup-command copy, and visible prototype feedback. 4. Treat `project-workspace.html` as the baseline for spacing and density; do not merge the new pages back into it. +@@ + | `desktop-remote-session.html` | jcode Desktop remote session view with running-on-device banner | ++| `workflow-contract.html` | Workflow Contract preview, built-in Agent Profile, readiness anatomy, and frozen execution manifest | ++| `review-coverage.html` | Deterministic PR Review commit pair, coverage ledger, validated findings, and frozen SCM grant | diff --git a/design/assets/workflow-contract.css b/design/assets/workflow-contract.css new file mode 100644 index 00000000..191e5590 --- /dev/null +++ b/design/assets/workflow-contract.css @@ -0,0 +1,130 @@ +.wc-shell { min-height: 100dvh; background: var(--surface); } +.wc-main { height: calc(100dvh - 3.25rem); overflow: hidden; } +.wc-head { display: flex; align-items: flex-start; justify-content: space-between; gap: 1.5rem; padding: 1rem 1.5rem; border-bottom: 1px solid var(--border); } +.wc-head h1 { margin: .14rem 0 .22rem; font-size: 1.18rem; letter-spacing: -.035em; } +.wc-head p { margin: 0; color: var(--text-muted); font-size: .7rem; } +.wc-head-actions { display: flex; align-items: center; gap: .6rem; } +.wc-layout { display: grid; height: calc(100% - 5.6rem); grid-template-columns: minmax(0, 1fr) 20rem; } +.wc-stage { min-width: 0; overflow-y: auto; padding: clamp(1.5rem, 4vw, 3.2rem); } +.wc-content { width: min(100%, 48rem); margin-inline: auto; } +.wc-intro { display: grid; grid-template-columns: 1fr auto; gap: 1.2rem; align-items: end; } +.wc-intro h2 { margin: .2rem 0; font-size: 1.55rem; letter-spacing: -.045em; } +.wc-intro p { max-width: 35rem; margin: 0; color: var(--text-dim); font-size: .78rem; line-height: 1.55; } +.wc-step { display: grid; gap: .15rem; text-align: right; color: var(--text-faint); font: .62rem var(--font-mono); } +.wc-step strong { color: var(--accent); font-size: .72rem; } +.wc-composer { margin-top: 1.4rem; border: 1px solid var(--border-strong); border-radius: .9rem; background: var(--surface-raised); box-shadow: var(--shadow); overflow: hidden; } +.wc-compose-body { padding: 1rem; } +.wc-compose-body textarea { box-sizing: border-box; width: 100%; min-height: 7rem; padding: .25rem; border: 0; outline: 0; resize: vertical; background: transparent; color: var(--text); font: .84rem/1.65 var(--font-sans); } +.wc-controls { display: flex; flex-wrap: wrap; align-items: center; justify-content: space-between; gap: .65rem; padding: .72rem .8rem; border-top: 1px solid var(--border); background: var(--surface-soft); } +.wc-control-set { display: flex; flex-wrap: wrap; gap: .45rem; } +.wc-pill { display: inline-flex; align-items: center; gap: .35rem; min-height: 1.8rem; padding: 0 .62rem; border: 1px solid var(--border); border-radius: 999px; background: var(--surface); color: var(--text-dim); font: 600 .66rem var(--font-sans); } +.wc-pill svg { width: .78rem; height: .78rem; } +.wc-pill[data-accent] { border-color: color-mix(in srgb, var(--accent) 36%, var(--border)); background: var(--accent-soft); color: var(--accent); } +.wc-planned { color: var(--text-faint); font-size: .58rem; font-weight: 500; } +.wc-readiness { margin-top: 1rem; border: 1px solid var(--border); border-radius: .8rem; background: var(--surface); overflow: hidden; } +.wc-readiness-head { display: flex; align-items: center; justify-content: space-between; gap: 1rem; padding: .85rem 1rem; cursor: pointer; } +.wc-readiness-title { display: flex; align-items: center; gap: .58rem; } +.wc-readiness-title > span:first-child { display: grid; width: 1.55rem; height: 1.55rem; place-items: center; border-radius: 50%; background: var(--success-bg); color: var(--success); font-weight: 800; } +.wc-readiness-title strong,.wc-readiness-title small { display: block; } +.wc-readiness-title strong { font-size: .73rem; } +.wc-readiness-title small { margin-top: .08rem; color: var(--text-muted); font-size: .62rem; } +.wc-disclosure { border: 0; background: transparent; color: var(--text-muted); font-size: .65rem; cursor: pointer; } +.wc-checks { display: grid; grid-template-columns: repeat(3, minmax(0,1fr)); border-top: 1px solid var(--border); } +.wc-check { display: grid; grid-template-columns: auto 1fr; gap: .5rem; padding: .75rem 1rem; } +.wc-check + .wc-check { border-left: 1px solid var(--border); } +.wc-check > span { color: var(--success); } +.wc-check strong,.wc-check small { display: block; } +.wc-check strong { font-size: .65rem; } +.wc-check small { margin-top: .12rem; color: var(--text-faint); font-size: .58rem; line-height: 1.45; } +.wc-contract-preview { margin-top: 1.7rem; } +.wc-section-head { display: flex; align-items: center; justify-content: space-between; gap: 1rem; } +.wc-section-head h3 { margin: 0; font-size: .9rem; } +.wc-section-head span { color: var(--text-faint); font: .6rem var(--font-mono); } +.wc-manifest { display: grid; grid-template-columns: repeat(2,minmax(0,1fr)); gap: .65rem; margin-top: .72rem; } +.wc-manifest-card { min-width: 0; padding: .85rem; border: 1px solid var(--border); border-radius: .65rem; background: var(--surface-raised); } +.wc-manifest-card > span { color: var(--text-faint); font: .57rem var(--font-mono); letter-spacing: .08em; text-transform: uppercase; } +.wc-manifest-card strong { display: block; margin-top: .28rem; font-size: .72rem; } +.wc-manifest-card p { margin: .18rem 0 0; color: var(--text-muted); font-size: .62rem; line-height: 1.5; } +.wc-inspector { min-height: 0; overflow-y: auto; border-left: 1px solid var(--border); background: var(--surface-soft); } +.wc-inspector section { padding: 1.15rem; } +.wc-inspector section + section { border-top: 1px solid var(--border); } +.wc-inspector h2 { margin: 0 0 .7rem; color: var(--text-faint); font: .6rem var(--font-mono); letter-spacing: .09em; text-transform: uppercase; } +.wc-inspector dl { display: grid; gap: .65rem; margin: 0; } +.wc-inspector dl div { display: grid; grid-template-columns: 5.3rem minmax(0,1fr); gap: .4rem; } +.wc-inspector dt { color: var(--text-faint); font-size: .61rem; } +.wc-inspector dd { margin: 0; color: var(--text-dim); font-size: .64rem; overflow-wrap: anywhere; } +.wc-inspector code { font-size: .59rem; } +.wc-contract-stamp { display: flex; gap: .7rem; align-items: center; padding: .72rem; border: 1px dashed var(--border-strong); border-radius: .6rem; } +.wc-contract-stamp > span { display: grid; width: 2rem; height: 2rem; place-items: center; border-radius: .5rem; background: var(--accent); color: var(--surface); font: 800 .66rem var(--font-mono); } +.wc-contract-stamp strong,.wc-contract-stamp small { display: block; } +.wc-contract-stamp strong { font-size: .7rem; } +.wc-contract-stamp small { margin-top: .1rem; color: var(--text-faint); font-size: .58rem; } +.wc-legend { display: grid; gap: .55rem; } +.wc-legend div { display: grid; grid-template-columns: .6rem 1fr; gap: .5rem; color: var(--text-muted); font-size: .62rem; } +.wc-legend i { width: .48rem; height: .48rem; margin-top: .18rem; border-radius: 50%; background: var(--success); } +.wc-legend div:nth-child(2) i { background: var(--accent); } +.wc-legend div:nth-child(3) i { background: var(--text-faint); } +.wc-hidden { display: none; } + +.rv-layout { display: grid; height: calc(100% - 5.6rem); grid-template-columns: minmax(0, 1fr) 20rem; } +.rv-main { min-width: 0; overflow-y: auto; padding: clamp(1.4rem, 3vw, 2.5rem); } +.rv-content { width: min(100%, 52rem); margin-inline: auto; } +.rv-revision { display: grid; grid-template-columns: 1fr auto 1fr; gap: .7rem; align-items: center; padding: .85rem 1rem; border: 1px solid var(--border); border-radius: .7rem; background: var(--surface-raised); } +.rv-revision div { min-width: 0; } +.rv-revision div:last-child { text-align: right; } +.rv-revision span,.rv-revision small { display: block; } +.rv-revision span { color: var(--text-faint); font: .56rem var(--font-mono); text-transform: uppercase; } +.rv-revision code { display: block; margin-top: .25rem; color: var(--text); font-size: .72rem; } +.rv-revision small { margin-top: .12rem; color: var(--text-muted); font-size: .58rem; } +.rv-arrow { color: var(--accent); font-weight: 800; } +.rv-coverage { margin-top: 1rem; border: 1px solid var(--border-strong); border-radius: .8rem; background: var(--surface); overflow: hidden; } +.rv-coverage-head { display: flex; align-items: flex-start; justify-content: space-between; gap: 1rem; padding: 1rem; } +.rv-coverage-head h2 { margin: .15rem 0; font-size: .9rem; } +.rv-coverage-head p { margin: 0; color: var(--text-muted); font-size: .64rem; } +.rv-state { border-radius: 999px; padding: .28rem .55rem; background: var(--success-bg); color: var(--success); font: 700 .6rem var(--font-mono); } +.rv-metrics { display: grid; grid-template-columns: repeat(3,1fr); border-top: 1px solid var(--border); } +.rv-metrics div { padding: .85rem 1rem; } +.rv-metrics div + div { border-left: 1px solid var(--border); } +.rv-metrics strong,.rv-metrics span { display: block; } +.rv-metrics strong { font: 650 1.25rem var(--font-mono); } +.rv-metrics span { margin-top: .12rem; color: var(--text-faint); font-size: .59rem; } +.rv-files { margin-top: 1.3rem; } +.rv-file-list { margin-top: .65rem; border: 1px solid var(--border); border-radius: .7rem; background: var(--surface-raised); overflow: hidden; } +.rv-file { display: grid; grid-template-columns: minmax(0,1fr) auto auto; gap: .8rem; align-items: center; padding: .72rem .85rem; } +.rv-file + .rv-file { border-top: 1px solid var(--border); } +.rv-file code { min-width: 0; overflow: hidden; text-overflow: ellipsis; font-size: .66rem; white-space: nowrap; } +.rv-file span { color: var(--text-faint); font-size: .6rem; } +.rv-file b { color: var(--success); font-size: .6rem; } +.rv-result { margin-top: 1.3rem; padding: 1rem; border: 1px solid var(--border); border-radius: .7rem; background: var(--surface-raised); } +.rv-result-head { display: flex; align-items: center; justify-content: space-between; gap: 1rem; } +.rv-result h2 { margin: 0; font-size: .9rem; } +.rv-finding { display: grid; grid-template-columns: auto 1fr; gap: .7rem; margin-top: .9rem; padding-top: .9rem; border-top: 1px solid var(--border); } +.rv-priority { height: max-content; border-radius: .3rem; padding: .18rem .34rem; background: var(--danger-bg); color: var(--danger); font: 700 .58rem var(--font-mono); } +.rv-finding strong,.rv-finding p,.rv-finding code { display: block; } +.rv-finding strong { font-size: .72rem; } +.rv-finding code { margin-top: .16rem; color: var(--accent); font-size: .6rem; } +.rv-finding p { margin: .35rem 0 0; color: var(--text-dim); font-size: .68rem; line-height: 1.55; } +.rv-tabs { display: flex; gap: .25rem; } +.rv-tabs button { border: 1px solid transparent; border-radius: .4rem; padding: .32rem .5rem; background: transparent; color: var(--text-muted); font-size: .62rem; cursor: pointer; } +.rv-tabs button[aria-selected="true"] { border-color: var(--border); background: var(--surface-raised); color: var(--text); } + +@media (max-width: 880px) { + .wc-layout,.rv-layout { grid-template-columns: minmax(0,1fr); height: auto; } + .wc-main { height: auto; overflow: visible; } + .wc-inspector { border-top: 1px solid var(--border); border-left: 0; } +} +@media (max-width: 620px) { + .wc-head { display: grid; padding: .85rem 1rem; } + .wc-stage,.rv-main { padding: 1.15rem 1rem; } + .wc-intro { grid-template-columns: 1fr; } + .wc-step { display: none; } + .wc-checks,.wc-manifest { grid-template-columns: 1fr; } + .wc-check + .wc-check { border-top: 1px solid var(--border); border-left: 0; } + .wc-controls { align-items: stretch; } + .wc-control-set { width: 100%; } + .rv-revision { grid-template-columns: 1fr; } + .rv-revision div:last-child { text-align: left; } + .rv-arrow { transform: rotate(90deg); } + .rv-file { grid-template-columns: minmax(0,1fr) auto; } + .rv-file b { grid-column: 1 / -1; } +} diff --git a/design/assets/workflow-contract.js b/design/assets/workflow-contract.js new file mode 100644 index 00000000..a7bcc8c1 --- /dev/null +++ b/design/assets/workflow-contract.js @@ -0,0 +1,20 @@ +document.addEventListener('DOMContentLoaded', () => { + const disclosure = document.querySelector('[data-contract-disclosure]'); + const body = document.querySelector('[data-contract-body]'); + disclosure?.addEventListener('click', () => { + const expanded = disclosure.getAttribute('aria-expanded') === 'true'; + disclosure.setAttribute('aria-expanded', String(!expanded)); + if (body) body.classList.toggle('wc-hidden', expanded); + disclosure.textContent = expanded ? 'Show scope' : 'Hide scope'; + }); + + document.querySelectorAll('[data-review-tab]').forEach((button) => { + button.addEventListener('click', () => { + document.querySelectorAll('[data-review-tab]').forEach((item) => item.setAttribute('aria-selected', String(item === button))); + const target = button.getAttribute('data-review-tab'); + document.querySelectorAll('[data-review-panel]').forEach((panel) => { + panel.classList.toggle('wc-hidden', panel.getAttribute('data-review-panel') !== target); + }); + }); + }); +}); diff --git a/design/review-coverage.html b/design/review-coverage.html new file mode 100644 index 00000000..84c6d72f --- /dev/null +++ b/design/review-coverage.html @@ -0,0 +1,42 @@ + + + + + + + + + + + + + Review coverage — jcode Cloud design + + + +
+
Projects/jcode/Review run 91bd60a
sample fixture · R1
+
+
jcode · Pull request review

Review credential snapshot convergence

GitHub PR #24 · synchronized event · 4 minutes ago

Posted
+
+
+
+
Event base7d37301main · exact commit
→
Event head0aa42e9fix/scm-grant · exact commit
+
+
Review plan · review-v2 · merge base 4fa21c0

Partial input coverage

Merge-base differs from the event base. Every eligible text hunk was indexed; one binary file is explicitly outside line review.

PARTIAL
+
13changed files
36 / 36indexed hunks
204changed lines
+
+

Coverage ledger

+
orchestrator/internal/api/source.go4 hunks · 31 linesindexed
orchestrator/internal/reconciler/reconciler.go8 hunks · 64 linesindexed
orchestrator/internal/store/plugins.go5 hunks · 42 linesindexed
Showing 3 of 12 eligible filesPublic summary omits changed-line rangesbounded
+
docs/architecture.pngbinary · no line anchorreported
+
+

2 validated findings

All anchors are changed lines
P1
Source path still re-resolves the current Service credentialorchestrator/internal/api/source.go:148 · 96% confidence

The source bundle can be created with a reconnect-time identity while review writeback uses the launch snapshot. Resolve both through the same Run SCM Grant.

P2
Repository rename can redirect a pending deliveryorchestrator/internal/reconciler/reconciler.go:1009 · 91% confidence

Owner/repository is loaded from the mutable Service at writeback. Use the frozen repository path stored with the Run.

+

Result state contract

separate fixtures required
No findingsValid result; coverage may still be partialresult
Plan unavailableLegacy Run; never labelled deterministiclegacy
Invalid outputRejected anchor remains a visible failureblocked
+
+
+ +
+
+
+ + diff --git a/design/workflow-contract.html b/design/workflow-contract.html new file mode 100644 index 00000000..8429145d --- /dev/null +++ b/design/workflow-contract.html @@ -0,0 +1,78 @@ + + + + + + + + + + + + + Workflow contract — jcode Cloud design + + + +
+
+
Projects/jcode/Tasks
+
sample fixture · Ship-R1 honest scope
+
+
+
+
jcode · main

Start a task

One Run, one frozen execution contract.

+
Service ready
+
+
+
+
+
+
New run

Tell jcode what outcome you need

The role guides how the agent works. Cloud freezes the trigger, model, delivery and verification before anything starts.

+
WORKFLOW CONTRACT01 / BUILT-IN
+
+
+
+
+
+ Developer · r1 selector in R2 + Isolated · no provider credential +
+
mainGLM-5.2 · high
+
+
+ +
+
+
W1
Contract preview · not a readiness verdictShip-R1 freezes actual facts on Run creation. Live capability preflight arrives in Ship-R2.
+ +
+
+
—
Effective timeout12 hours from current Cluster default; the Run and Kubernetes deadline must match
+
—
Typed deliverycreate_pull_request only · lifecycle-aware · never auto-merge
+
→
Future readinessRunner manifest and toolchain blockers are deliberately not claimed in Ship-R1
+
+
+ +
+

What will be frozen

schema v1
+
+
RoleDeveloper · revision 1

Edit the repository, verify observable behavior, and deliver a reviewable change.

+
TriggerManual · Jack

Direct member request from the jcode Service workspace.

+
DeliveryReady Pull Request

One-shot success publishes Ready. Draft is only a long-session intermediate state.

+
VerificationRepository-owned tests

Ship-R1 records the declared verification and actual result. Toolchain preflight arrives in Ship-R2.

+
+
+
+
+ +
+
+
+ + diff --git a/docs/11-api.md b/docs/11-api.md index 4037acd3..bb30c3be 100644 --- a/docs/11-api.md +++ b/docs/11-api.md @@ -1280,6 +1280,35 @@ runner 通过 `POST /internal/v1/runs/{id}/events` 上报;orchestrator 也会内 > `agent.permission_request` 的 upsert 失败时**整批回 5xx**(runner 重发幂等), > 绝不 ack 一个没落库的请求事件。 +### 5.6 `GET /internal/v1/runs/{id}/source` — 获取冻结 SCM 源码 + +仅 provider Service 使用。Run 在调度领取时把 provider 配置版本、凭据版本、 +repository id/path/clone URL/default branch 和 acting principal 一起冻结到 +`run_plugin_snapshots`。本端点只用该快照签发短期凭据并生成无凭据 Git bundle； +领取后发生的仓库改名、Service 重绑或 Plugin 重连不会改变既有 Run 的源码目标。 +历史 Run 没有 repository grant 时保留旧解析路径；新 Run 的冻结 grant 无法签发凭据 +则返回 `409 scm_grant_unavailable`，绝不回退到别的用户或集群身份。 + +### 5.7 `POST /internal/v1/runs/{id}/review-plan` — 冻结确定性评审输入 + +仅 `kind=review` 且处于 `scheduling|running` 的 Run；请求体为单个严格 JSON 对象： + +```json +{ + "base_sha": "1111111111111111111111111111111111111111", + "head_sha": "2222222222222222222222222222222222222222", + "merge_base_sha": "1111111111111111111111111111111111111111", + "diff": "diff --git a/main.go b/main.go\n..." +} +``` + +服务端核对 Run 已冻结的 base/head，解析右侧新增行 anchor，丢弃 raw diff，只持久化 +canonical `review_plan`。首次写入 `201`，相同 plan 重试 `200`，不同 plan +`409 review_plan_conflict`，revision 不符 `409 review_revision_mismatch`，超过 +2 MiB/400 files/2000 hunks 返回 `413 review_input_too_large`。公开 Run JSON 只返回 +coverage 与逐文件计数，不返回私有 anchor；随后结构化 Review Result 的每个 finding +都必须命中该计划的 changed right-side line。 + --- ## 6 · Runner Job 环境变量(runner-integration agent 对接清单) diff --git a/docs/28-workflow-contract-and-deterministic-review-prd.md b/docs/28-workflow-contract-and-deterministic-review-prd.md new file mode 100644 index 00000000..48448c6f --- /dev/null +++ b/docs/28-workflow-contract-and-deterministic-review-prd.md @@ -0,0 +1,410 @@ +# 28 · Workflow Contract、Runner Readiness 与确定性 Review PRD + +状态：Ready for Ship-R1 implementation +优先级：P0 +父级研究：`outputs/cloud-workflow-dogfood-report.md`（工作区交付物） +开源对标：`docs/30-open-source-agent-workflow-benchmark.md` +Published issue: https://github.com/cnjack/cloud/issues/24 + +## Problem + +jcode Cloud 已经能从 Console、jtype Card、SCM event/comment、Cron 与 scoped +API 创建 Run，也已经具备 Plugin、provenance、Automation execution ledger、 +Usage 和 lifecycle-aware Pull Request delivery。最新主线解决了“能不能执行” +的大部分问题，但还不能可靠回答“这一次究竟按哪份工作流执行”。 + +当前执行语义分散在 Run 标量字段、Automation prompt/model、Service policy、 +Plugin snapshot、Runner shell 与 hard-coded review protocol 中。Profile 或 +Automation 改动后，用户无法从历史 Run 看出它采用了哪个版本；retry、resume、 +webhook 与 poller 也容易只复制部分字段。Agent 只能从 prompt 和当前环境猜测 +角色、交付和验证要求。 + +私有仓库路径也没有完全冻结为同一项授权事实。Runner clone 已有 run-scoped +credential substrate，Plugin Review writeback 已部分使用 immutable snapshot， +但 source bundle、push、PR lifecycle 和 legacy fallback 仍有读取当前 Service/ +binding/credential 的 implementation。同一个 Run 可能在开始和交付阶段看到 +不同的 repository identity 或 grant revision。 + +PR Review 已有 readonly Runner、严格 JSON、最多 8 条 finding、80% confidence +门槛和 GitHub inline fallback，但仍以 branch 名而不是 commit SHA 定位输入， +没有 changed-hunk index、rule revision 或 coverage ledger。大 PR 仍是一次把整份 +diff 交给模型；finding 的行号格式合法不等于确实锚定本次改动。 + +Runner image 当前明确只保证 git、curl、zstd 与 ripgrep，不带 Go、Node、Python +或 Java toolchain。Cloud 没有 Runner Profile catalog，也没有在排队前把 Agent +Profile requirements 与真实 Runner capability 比对。长达三十分钟的任务可能 +在启动后才发现无法测试或缺少 MCP/Plugin capability。 + +这些问题直接影响四类核心场景： + +1. PR Review 需要可复现输入、完整覆盖和可信行锚点； +2. PM/竞品研究需要明确的 MCP 与 Work Item 写入 capability； +3. jtype Card pickup 需要固定的 Developer profile、交付和完成条件； +4. Cron repo maintenance 需要固定的时间窗口、验证命令、Changelog/Report 输出。 + +## Solution + +Cloud 建立一条统一但不抹平 trigger 差异的派发链： + +> Trigger adapter 只提供来源事实；Workflow Contract Resolver 把 definition、 +> facts、Agent Profile revision、Service policy、模型、交付和验证冻结成一个 +> contract；Runner 只消费冻结 contract。Ship-R2 再把现有 dispatch prerequisites +> 收敛为可 preview 的单一 Run Readiness evaluator。 + +首个 vertical slice 以私有 GitHub PR Review 为验收主线，同时交付 Manual +任务与 Review Run 可见的 contract/coverage inspector。它包含三个相互依赖的深模块： + +1. **Workflow Contract Snapshot**：每个 Run 冻结 schema version、workflow、profile + revision、trigger、execution、delivery、verification 和 capability requirement； +2. **Run SCM Grant**：在派发 claim 时冻结安装、凭据版本、provider config、 + repository identity、clone route、default branch 和 acting identity；所有 + source/PR/review implementation 只消费它； +3. **Review Plan / Coverage**：Review Run 单独固定 base/head SHA，建立 changed-hunk + index，finding 必须命中右侧 changed line；首期保留单模型执行，但持久化 + coverage，为后续 bounded shards 留下稳定 seam。 + +Ship-R1 不交付 Runner Profile Catalog、Composer live readiness 或 Custom Profile +CRUD。合同中的 timeout 直接冻结当前 Project override 或 Cluster default 的实际 +effective value，必须与 Kubernetes `activeDeadlineSeconds` 一致。Ship-R2 引入 +operator-owned Runner manifest 和单一 evaluator，再检查 toolchain/capability 并在 +创建前返回 typed blocker；任何阶段都不得静默换模型、换 profile、改 output 或降级 +为匿名 clone。 + +开源对标后，产品采用明确的“可编辑定义 → 编译合同”分层。R2 的 Workflow +Definition 可由 Console conversation、表单或 repository-owned +`.jcode/workflows/*.md` 创建，使用同一 versioned schema；publish 时编译/验证并产生 +新 revision。每个 Ship-R1 Run 已先冻结 `workflow.id/revision/source/definition_hash`， +因此 Agent 知道 workflow 不是靠猜 prompt，而是 Orchestrator 注入这份平台拥有的 +合同。定义变更只影响未来 Run，retry 默认仍使用旧合同。 + +### 为什么不同 case 不需要四套 Workflow engine + +Manual、JType、SCM 与 Cron 的 trigger、幂等和外部回写确实不同，因此保留各自 +adapter 和 occurrence/receipt；它们在创建 Run 前收敛到相同的 contract resolver， +并在 Ship-R2 收敛到 readiness interface。这样 trigger 差异是一等事实，但角色、能力、交付和审计 +不会各写一套。 + +### Draft PR decision + +不再把“草稿 PR”作为新 Service 的产品默认或 workflow 类型。Pull Request 是 +delivery；one-shot 成功直接 Ready，长 session 可在执行期间保持 Draft，并在 +最终 bundle 推送后转 Ready。`always_draft` 仅作为历史兼容和 Owner 显式策略， +Cloud 仍不自动 approve、merge 或主动 dispatch CI。 + +### Multi-agent decision + +Developer、PM、Architect、Reviewer 首先是不同 Agent Profile，不要求多 Agent。 +首期每个 Run 仍只有一个 primary agent。只有确定性 Review shards、互不重叠的 +work units 或显式“提出方案—独立审计—合并结论”工作流才允许受控 fan-out； +没有隔离、budget、merge policy 和 loop limit 时不启用多 Agent。 + +## User Stories + +### Profile 与 Workflow + +1. 作为 Project Member，我希望在发起任务前选择 Developer、PM、Architect 或 + Custom profile，以便模型获得明确角色，而不是从自然语言猜测。 +2. 作为 Project Owner，我希望创建 Custom profile 并声明 instructions 与 + required capabilities，以便团队复用经过评审的工作方式。 +3. 作为 Project Owner，我希望编辑 profile 产生新 revision，以便历史 Run + 继续显示旧版本。 +4. 作为 Project Member，我希望在发送前看到将被冻结的 trigger、profile、 + model、permission、delivery 和 verification 摘要，以便确认系统理解正确。 +5. 作为 Viewer，我希望在 Run detail 查看完整 Workflow Contract 和 hash，以便 + 审计一次执行为何这样工作。 +6. 作为 Automation Author，我希望 SCM/Cron Automation 固定一个 profile + revision；后续编辑 profile 不改变已发生的 execution。 +7. 作为 Reviewer，我希望 retry 明确标记“复用原 contract”或“按当前配置重新 + 解析”，避免半旧半新的执行。 +8. 作为安全审计者，我希望 profile instructions、accountable actor 与 Bot 都不 + 参与 credential selection 或 authorization。 + +### Readiness + +9. 作为 Project Member，我希望排队前知道 source、model、Runner、Plugin、 + workspace 和 delivery 是否就绪，以免长任务启动后才失败。 +10. 作为 PM profile 用户，我希望缺少 MCP fetch 或 jtype create-card capability + 时看到明确 blocker 和配置入口，而不是 Agent 假装完成研究/入板。 +11. 作为 Developer，我希望 profile 要求 Node/Go/Python 时系统核对 Runner + manifest 的工具及版本，以免“实现完成但无法测试”。 +12. 作为 Cluster Admin，我希望 Runner manifest 包含 image digest、architecture、 + tool versions 和 capability，以便 rollout 后可追踪实际执行环境。 +13. 作为 Project Member，我希望 preview 与 Create Run 使用同一 evaluator， + 以免 UI 显示 Ready 但提交后走另一套判断。 +14. 作为 Operator，我希望 manifest 漂移在 init preflight 中 fail closed，并把 + 稳定原因写回 Run，而不是继续运行未知镜像。 + +### SCM grant 与 Review + +15. 作为私有仓库 Owner，我希望一个 Run 的 clone、push、开 PR、读取 PR 状态 + 和 review writeback 使用同一 installation/grant revision。 +16. 作为 Project Owner，我希望 Run 开始后即使 Plugin reconnect、repository + rename 或 Service setting 改变，本 Run 仍按冻结 grant 完成或明确失败。 +17. 作为安全审计者，我希望 Runner 永远看不到 provider token，grant 仅由 + control-plane adapter 消费。 +18. 作为 PR Author，我希望 Review 显示实际 base SHA、head SHA 与 rules revision， + 以便结果可复现。 +19. 作为 PR Author，我希望 inline finding 只能落在本次 diff 的右侧 changed + line，以免评论锚到无关或过期代码。 +20. 作为 PR Author，我希望看到 indexed files、input-covered hunks 与 skipped reason， + 以免“0 finding”被误解成“全量检查无问题”。 +21. 作为 Reviewer，我希望 provider 不支持 inline placement 时仍收到包含精确 + path/line 的单条 summary fallback。 +22. 作为 Automation Author，我希望 synchronized 事件产生的新 Run 使用新的 + head SHA，而同一 delivery replay 仍只产生一次 Run。 + +### 操作与恢复 + +23. 作为 Operator，我希望 contract hash、Runner digest、SCM grant revision 和 + review coverage 出现在 Run inspector，而 secrets 永不出现。 +24. 作为 Project Member，我希望任务的 effective timeout 在 Run 开始前可见，并能 + 区分“排队阻塞”和“执行失败”；POC workflow 的 timeout 不得低于 1800 秒。 +25. 作为未来 Workspace recovery 用户，我希望 contract 与 SCM grant 可作为 + checkpoint lineage 的稳定父引用，避免恢复到另一套工作流。 +26. 作为 jcode 编辑工具维护者，我希望 Cloud 不假装通过 turn-hook 解决同路径 + lost update；CAS/atomic mutation 留在实际文件 mutation seam。 + +## Product Requirements + +### PR-1 · Versioned Agent Profiles + +- Built-in profile 有稳定 ID、版本和只读 instructions/requirements； +- Custom profile 为 Project scope，Owner 可创建、编辑、归档； +- 编辑产生新 revision，旧 revision 只读保留； +- instructions 有长度上限，不允许包含 secret； +- required capability 使用受控枚举，不接受任意“看起来像能力”的字符串； +- role 不影响 RBAC、provider identity 或 Cloud principal。 + +### PR-2 · Immutable Workflow Contract + +- 所有新 Run 在进入 queued 前拥有 contract snapshot； +- snapshot 至少包含 schema version、workflow revision、profile revision、trigger、execution、 + delivery、verification、requirements、resolved-at 和 stable hash； +- snapshot 是 API 可读、Store append-only 的审计事实； +- Runner 接收 contract-derived instructions 与 metadata，不读取 mutable profile； +- legacy Run 明确显示 `contract unavailable`，不伪造高精度 snapshot； +- retry/resume policy 必须显式，首期 retry 默认复用原 contract；来自新 trigger + 的新 Run 总是解析新 contract。 +- execution 冻结当前实际 effective timeout；首批 POC workflow 要求该值至少 + 1800 秒，但 1800 不是另一套默认值。Ship-R2 超过 Runner Profile 上限时 readiness + 阻塞，不在执行中静默截断； +- delivery 是 allowlisted typed outputs，Agent 不可增加合同之外的写操作。 + +### PR-3 · Single Run Readiness Evaluator + +- Preview 和 Create Run 共用 evaluator； +- 每项 check 返回 `ready | blocked | unavailable`、stable code、message、repair role； +- required check blocked 时不得创建 Run； +- optional capability 缺失可以显示 degraded，但不得改变 delivery/output； +- Runner manifest 由 operator 配置/镜像证明，浏览器不得自行推断； +- capability 版本比较确定且有单元测试。 + +### PR-4 · Frozen Run SCM Grant + +- grant 与 dispatch claim 原子创建； +- grant 引用 immutable provider config/credential version 并冻结 repository facts； +- source bundle、push、PR create/readiness、review read/write 全部消费 grant； +- 当前 Service/binding 只用于新 Run，不得改变现有 grant； +- legacy fallback 必须显式标记，Plugin-bound 新 Run 不允许 fallback； +- terminal secret material 依照现有 GC policy 清理，但 audit identifiers 保留。 + +### PR-5 · Deterministic Review Plan + +- Review input 固定 base/head commit SHA；branch 只作为展示来源； +- changed-hunk index 在模型调用前生成并持久化摘要； +- Review result validation 校验 path、right-side changed line、finding 数量、 + confidence 与字段长度； +- Ship-R1 coverage 是 input coverage，至少记录 changed files、eligible/indexed + hunks、skipped files/hunks 和 reason；不声称模型语义上审阅了每个 hunk； +- `0 findings` 与 `incomplete coverage` 可同时成立，UI 必须区分； +- provider adapter 只接受 validated result，不理解 prompt。 + +### PR-6 · UI Contract + +- Ship-R1 Composer 只显示 read-only built-in Profile pill 和合同说明，不显示假的 + readiness；Ship-R2 才提供 Profile selector 与 live Readiness strip； +- Run detail 的 Workflow 区域先显示 Profile、Trigger、Delivery、Verification， + technical detail 再显示 hash、Runner digest、SCM grant revision； +- Review Run 显示 coverage bar、base/head short SHA、rules revision 与 skipped reason； +- blocked 用持久 panel，不以 Toast 替代； +- Ship-R1 键盘可展开 contract；Profile selector 与 preflight 交互属于 Ship-R2； +- 700px 以下所有事实单列，核心 blocker 在 technical identifiers 之前。 + +### PR-7 · Portable Workflow Definition（Ship-R2 authoring） + +- Console、conversation-assisted setup 与 repository file 使用同一 schema； +- file authoring 使用 Markdown frontmatter + prompt body，默认路径 + `.jcode/workflows/*.md`； +- schema 包含 trigger、profile、requirements、timeout、allowed outputs 与 prompt； +- validate/publish 产生新 immutable revision，invalid revision 不替换 last known good； +- import 创建 disabled draft，必须 preview/readiness 后由 Owner 启用； +- 首批提供 PR Review、JType Developer pickup、Competitor research → JType、Weekly + repository report/changelog 四个 proven workflows； +- 不把 provider-specific GitHub Actions lock file 当跨 provider runtime contract。 + +### Ship matrix + +| Product requirement | Ship-R1 | Ship-R2+ | +| --- | --- | --- | +| PR-1 Agent Profiles | code-owned Developer/Reviewer r1 | PM/Architect、Custom CRUD/revisions | +| PR-2 Workflow Contract | all new Run paths、built-in definitions、immutable snapshot | custom/portable definition revisions | +| PR-3 Readiness | existing dispatch prerequisites only | shared preview/create evaluator、Runner manifest/catalog | +| PR-4 SCM Grant | repository + provider + credential convergence | additional provider policy/capability evidence | +| PR-5 Review Plan | exact revision pair、input coverage、anchor validation | deterministic shards、semantic review coverage | +| PR-6 UI | Run contract inspector、Review input coverage | selector、live readiness、authoring UI | +| PR-7 Definition | code-owned `builtin:implementation-task@1` and `builtin:pull-request-review@1` | UI/repository authoring and import/export | + +## Implementation Decisions + +### Deep module 1：Workflow Contract Resolver + +Resolver 接收已经授权且规范化的 trigger facts、Agent Profile revision、Service +policy、model selection 与请求选项，返回有界、可哈希的 contract。Manual、 +JType、SCM、Cron adapter 不复制默认值和 snapshot 逻辑。Contract canonicalization +固定字段顺序和空值规则，使 hash 可重复。 + +Profile instructions 不是 system authority。Resolver 用平台拥有的 envelope 把 +profile 文本标记为 role guidance；Delivery Contract、security invariant 和 Review +protocol 由平台层追加且不可被 profile 覆盖。 + +### Deep module 2：Run Readiness Evaluator（Ship-R2） + +Evaluator 消费 contract 与当前 capability evidence，输出有序 checks。它不执行 +provider mutation，不创建 Run，也不改变输入。Create Run 在同一请求中重新计算 +并比较 contract hash；preview 已过期时返回 `preflight_stale`，而不是沿用旧结论。 + +首期 capability taxonomy：source read/write、git、shell、ripgrep、persistent +workspace、SCM read/write/review、JType read/create-card/comment、MCP fetch，以及 +Go/Node/Python/Java toolchain。未来新增 capability 只能向后兼容。 + +### Deep module 3：Run SCM Grant + +Run SCM Grant 组合现有 Plugin runtime snapshot 与 repository snapshot。它是所有 +provider operation 的窄 interface。Control plane 可用 grant 签发短期 credential、 +构建 source bundle、推送 bundle、创建/读取 PR 与发布 Review；Runner 只看到 +source adapter 和结果，不看到 grant secret。 + +Create/preview 阶段只能检查 `scm_install_ready`，因为 Run SCM Grant 尚未存在。 +Grant 在 dispatch claim transaction 内冻结。Plugin-bound Run 若找不到匹配 grant +即 fail visible；只有明确标记的 legacy Run 才可走现有 compatibility resolver。 +已经 queued 的 Run 在 claim 时发现 drift，终止为 `dispatch_claim_unavailable`；它不 +使用新 binding 重试，也不保留一份看似有效的 partial grant。 + +### Deep module 4：Review Plan / Coverage Ledger + +Review Plan 把 immutable commit pair 转为 changed-hunk index 与 input coverage units。 +首期仍执行一个 model turn，但 result validator 根据 index 校验 anchor，并把 eligible +hunks 标记为 indexed 或 skipped；它不声称模型语义上逐个审阅。后续多 Agent/shard 只替换 plan executor，不改变 +provider adapter 或 result contract。 + +### Trigger adapter contract + +- Manual：direct actor、prompt、selected profile/model/branch/permission； +- JType：claim/occurrence、Card ref、external actor、Automation owner； +- SCM：verified receipt、provider actor、repository/ref/object/action； +- Cron：scheduled fire、rule owner、window、output mode； +- API key：scoped service principal、idempotency key。 + +每个 adapter 负责自己的 verification 与 idempotency，但必须在创建 Run 前调用 +同一个 resolver/evaluator。 + +### 开源对标约束 + +- 产品面以 OpenHands Agent Canvas 为主对标，明确拆分 Agent Profile、LLM Profile + 与 Runner Backend； +- Workflow authoring 与安全输出参考 GitHub Agentic Workflows，但执行合同保持 + provider-neutral； +- JType pickup 的 reconcile/retry/workspace lineage 参考 Symphony，并使用 Cloud + durable Store 而不是进程内状态； +- PR Review 的 file/hunk coverage、定位和幂等发布参考 OpenCodeReview； +- 不复制“把宽写权限 PAT 直接交给 Agent/MCP”的路径，所有外部写入仍由 control + plane grant + typed delivery adapter 完成。 + +## Testing + +### Contract tests + +1. Ship-R1 Developer/Reviewer 两种 code-owned built-in profile round-trip； +2. built-in definition 的语义内容改变但 revision 未递增时测试失败； +3. canonical contract hash 在 Memory/PostgreSQL 和重启后相同； +4. Manual/JType/SCM/Cron 经过各自 adapter 后共享同一个 resolver/canonicalization， + 但保留各自的 profile、delivery default 与 idempotency facts； +5. retry 明确复用 snapshot，new trigger 解析新 revision； +6. profile text 无法覆盖 Delivery Contract 或授权。 + +### Readiness tests（Ship-R2） + +1. 缺模型、branch、Plugin、SCM grant、Runner capability、persistent workspace； +2. optional 与 required capability 的差别； +3. preview/Create Run 共享 cases； +4. stale hash、Runner digest 漂移和 typed repair role； +5. PM profile 缺 MCP/JType、Developer profile 缺 toolchain 的负路径。 + +### SCM/review tests + +1. Plugin reconnect、Service repository rename、provider config change后，旧 Run + 仍消费原 grant； +2. source、push、PR、review 调用的 installation/config/credential/repository revision + 完全一致； +3. base/head branch 移动后 Review 仍使用 frozen SHA； +4. finding 命中 changed right-side line 才通过；context/deleted/out-of-range line 拒绝； +5. coverage complete、partial、skipped、0 finding； +6. GitHub inline batch 与 summary fallback； +7. duplicate synchronized delivery 不重复 Run，新 head SHA 创建新 Run； +8. publisher 稳定排序、bounded batch、retry 后不重复 inline/summary comment。 + +### UI tests + +1. Ship-R1 keyboard contract disclosure；Ship-R2 再覆盖 profile selection 与 preflight； +2. Ship-R1 legacy/partial/invalid/loading states；Ship-R2 再覆盖 readiness stale/unavailable； +3. Run contract inspector 与 legacy empty state； +4. Review coverage complete/partial 与窄屏； +5. Viewer 只读，Member dispatch；Owner profile mutation 属于 Ship-R2。 + +### Delivery proof + +- Orchestrator `go test ./...`； +- Console `pnpm test && pnpm typecheck`； +- Runner shell/Go tests与本地 mock review journey； +- 受影响 Kustomize target render； +- Copilot CLI（优先 claude-sonnet-5；不可用时明确记录实际模型，不静默冒充）、 + Claude CLI（glm-5.2）、Grok CLI 在设计前后和 implementation 后各独立审计； +- GitHub Ready PR、main merge、amd64 images workflow、`wangwenhui@local` 的 + `jcode` namespace rollout 与 live health/migration 验证。 + +## Out of Scope + +- 首期 visual DAG/workflow canvas； +- 默认启用多 Agent 或 Agent 自主互相委派； +- 任意第三方 prompt 变成 system instruction； +- 自动 approve、merge 或显式 dispatch repository CI； +- 把 Draft PR 删除为不兼容状态； +- 普通 Run 的中途 workspace checkpoint； +- 在 Cloud turn-hook 中模拟文件级 CAS； +- 首期并行 Review shards； +- 自动安装语言 toolchain；Ship-R2 缺少时由 readiness 阻塞； +- 复制 jtype Card/PR 为 Cloud 内部 Work Item。 + +## Further Notes + +### Superseded decisions + +- 早期 Plugin 文档中“Cloud 不负责 source/bundle/push/PR/comment”已被当前 + run-scoped source 与 control-plane delivery implementation supersede； +- D08 的 Draft PR 默认已被 lifecycle-aware delivery supersede； +- D19 legacy Integration 只作为兼容，不得成为 Plugin-bound 新 Run 的 fallback； +- D04 的 per-issue PVC 描述与实际 per-Service PVC 不一致；新设计使用“Service + workspace cache”，不把 PVC 描述成 Work Item checkpoint。 + +### Release sequence + +1. **Ship-R1 / 本 PR**：Workflow Contract snapshot、Run SCM Grant convergence、 + deterministic Review commit pair/anchor validation、Run inspector UI； +2. **Ship-R2**：Project Agent Profile revision CRUD、Runner Profile catalog、Composer + readiness preview、Portable Workflow Definition 与 Automation profile binding； +3. **Ship-R3**：Workspace Checkpoint + sibling jcode mutation CAS； +4. **Ship-R4**：bounded Review shards 与受控 multi-agent execution。 + +Ship-R1 是 tracer bullet，不把尚未实现的 Ship-R2 UI/CRUD 伪装成已交付。Ship-R1 必须证明 +同一个私有 GitHub Review Run 从 source 到 writeback 使用同一 frozen grant，且 +Run UI 可解释其 contract 与 review coverage。 diff --git a/docs/29-workflow-contract-and-review-design.md b/docs/29-workflow-contract-and-review-design.md new file mode 100644 index 00000000..b27cc7f6 --- /dev/null +++ b/docs/29-workflow-contract-and-review-design.md @@ -0,0 +1,680 @@ +# 29 · Workflow Contract 与 Deterministic Review 详细设计 + +状态：Ready for Ship-R1 implementation +产品合同：`docs/28-workflow-contract-and-deterministic-review-prd.md` +对标依据：`docs/30-open-source-agent-workflow-benchmark.md` +首个实现范围：Ship-R1 tracer bullet + +## 1. 设计目标 + +Ship-R1 必须在不引入通用 Workflow Builder 的前提下证明三件事： + +1. 任意新 Run 都有一份可展示、可哈希、不可变的 Workflow Contract； +2. Plugin-bound 私有 GitHub Review Run 从 source 到 review writeback 使用同一份 + frozen SCM grant； +3. Review 固定到 commit pair，所有 inline finding 通过 server-owned changed-line + index 校验，并在 Run detail 展示 coverage。 + +Ship-R1 不创建 Custom Agent Profile CRUD。它先提供两个 versioned built-in profiles： +Developer 与 Reviewer。PM、Architect、Custom profile 以及 Runner readiness picker +按同一 API/视觉合同在 R2 进入，不在 R1 伪造不可用动作。 + +对标调研进一步固定三个边界：OpenHands 式 Agent Profile、LLM Profile 与 Runner +Backend 是不同概念；Ship-R1 不新建 LLM Profile 或 Runner Profile CRUD，只冻结 +当前 model selection 与实际 effective timeout。`gh-aw` 式可编辑 Workflow +Definition 与每次运行的 compiled contract 是不同对象；任何 provider write 都是 +typed delivery，不由 Agent 持有宽写权限直接执行。Ship-R1 先实现 compiled +contract，Ship-R2 再开放 definition authoring、LLM/Runner profile 管理与 live +readiness。 + +## 2. Architecture + +```mermaid +flowchart LR + T["Trigger implementation"] --> C["Workflow Contract module"] + C --> R["Run + immutable contract"] + R --> D["Dispatch claim"] + D --> G["Run SCM Grant"] + G --> S["Source adapter"] + S --> P["Review plan ingest"] + P --> A["Agent review turn"] + A --> V["Review result validator"] + V --> W["Provider writeback adapter"] + G --> W +``` + +For an implementation Run, the only Ship-R1 delivery variant is: + +```json +{ + "delivery": { + "outputs": [{ + "type": "create_pull_request", + "target": "service_repository", + "ready_policy": "lifecycle_aware" + }], + "merge": "never" + } +} +``` + +`ready_policy` is required for `create_pull_request` and forbidden for +`provider_review`. A readonly Service uses `{"type":"diff_only"}` and carries +neither provider target nor lifecycle fields. + +### 2.1 Module responsibilities + +| Module | Owns | Does not own | +| --- | --- | --- | +| Workflow Contract | canonical snapshot、built-in profile revision、hash、Runner instruction envelope | authorization、credential、mutable profile lookup | +| Run SCM Grant | provider/config/credential/repository immutable references、credential issuance adapter | prompt、review rules、provider UI | +| Review Plan | commit pair、unified-diff parser、changed-line index、coverage summary | model execution、provider comment formatting | +| Review Result | structural limits + changed-line anchor validation | git diff creation、credential | +| Run inspector | present frozen contract/grant/coverage | derive current policy as historical truth | + +## 3. Data design + +Migration `0064_workflow_contract.sql` is additive and idempotent. It alters both +`runs` and `run_plugin_snapshots`; repository identity is not left in a separate +future migration. + +### 3.1 Run columns + +| Column | Type | Default | Meaning | +| --- | --- | --- | --- | +| `execution_contract` | JSONB | `{}` | canonical immutable contract; legacy rows remain empty | +| `pr_head_sha` | TEXT | `''` | exact reviewed/patched head commit | +| `pr_base_sha` | TEXT | `''` | exact base tip from the verified event/provider | +| `review_plan` | JSONB | NULL | first-writer-wins changed-line index + coverage metadata | + +`execution_contract` is written in the same transaction as the Run. `review_plan` +is written once by the Runner pre-agent bootstrap after the exact source bundle is +materialized. A later agent process holding the same Run token cannot overwrite it. + +### 3.2 Workflow Contract schema v1 + +```json +{ + "schema_version": 1, + "hash": "sha256:…", + "workflow": { + "id": "builtin:pull-request-review", + "name": "Pull Request Review", + "revision": 1, + "source": "builtin", + "definition_hash": "sha256:…" + }, + "profile": { + "id": "builtin:reviewer", + "name": "Reviewer", + "role": "reviewer", + "revision": 1, + "instructions": "Review the exact change…" + }, + "trigger": { + "kind": "scm", + "origin": "automation", + "receipt_id": "delivery-id", + "actor": "github:user:123", + "repository": "cnjack/cloud", + "object": "pull_request:24", + "action": "synchronize" + }, + "execution": { + "run_kind": "review", + "llm_selection": { + "model_id": "…", + "model_name": "…", + "effort": "high", + "source": "run_request" + }, + "session": false, + "permission_mode": "full_access", + "workspace_access": "read_only", + "provider_credentials": "none", + "base_ref": "main", + "timeout_seconds": 43200, + "timeout_source": "cluster_default" + }, + "delivery": { + "outputs": [{"type": "provider_review", "target": "trigger_pr"}], + "merge": "never" + }, + "verification": { + "mode": "structured_review", + "rules_revision": "review-v2", + "max_findings": 8, + "minimum_confidence": 80 + }, + "requirements": ["source.read", "git", "ripgrep", "scm.review.write"], + "resolved_at": "2026-08-01T00:00:00Z" +} +``` + +The hash is computed over the same object with `hash` and `resolved_at` omitted. +Go struct field order is the canonical order; unknown fields are rejected when a +contract is decoded from storage. Excluding `resolved_at` lets preview and create +compare the same resolved configuration. Run ID is the instance identity; +`workflow.definition_hash` and contract `hash` compare definition/configuration, +not occurrence identity. + +`workflow.definition_hash` is stable across Runs that use the same Workflow +revision. Ship-R1 built-in definitions are code-owned constants. Ship-R2 +definitions are published artifacts compiled from UI or +`.jcode/workflows/*.md`. The Runner receives the contract envelope, not a mutable +file lookup. Ship-R1 freezes the same Project/Cluster effective timeout already +used for Kubernetes `activeDeadlineSeconds`; it does not pretend to validate a +future Runner Profile. + +Ship-R1 built-ins are explicit: + +| Definition | Profile | Used for | Revision rule | +| --- | --- | --- | --- | +| `builtin:implementation-task@1` | `builtin:developer@1` | any non-Review Manual/JType/SCM/Cron/API Run without a published custom workflow | any semantic prompt/requirements/output change requires a revision bump | +| `builtin:pull-request-review@1` | `builtin:reviewer@1` | automatic/manual Pull Request Review | any protocol/rules/requirements/output change requires a revision bump | + +`definition_hash` is SHA-256 over the canonical built-in definition with name, +revision, profile reference, requirements, output types and platform-owned prompt +body. A changed hash without a revision bump fails a unit test. + +`llm_selection` is a frozen projection, not an LLM Profile revision. +`timeout_source` and `workspace_access` are resolved execution facts, not a +Runner Profile. Ship-R1 contract/API contains no `llm_profile_revision` or +`runner_profile_revision`; those fields may be added only when the corresponding +resources exist in Ship-R2. + +Ship-R1 `requirements` are a closed binary enum used for audit/instruction +derivation; they are not a claim that tool versions were preflighted. Ship-R2 +replaces toolchain entries with typed constraints such as +`{"capability":"node","minimum_version":"22"}` and compares them with the +operator-owned Runner manifest. + +The exhaustive Ship-R1 enum is `source.read`, `source.write`, `git`, `shell`, +`ripgrep`, `scm.pull_request.write`, and `scm.review.write`. The Developer +built-in requires `source.read`, `source.write`, `git`, `shell`, and `ripgrep`, +plus `scm.pull_request.write` only when delivery is `create_pull_request`. The +Reviewer built-in requires `source.read`, `git`, `ripgrep`, and +`scm.review.write`. No unrecognized string is accepted. + +### 3.2.1 Trigger facts by kind + +`trigger.kind` is a discriminant, not a three-field generic bag: + +| Kind | Required frozen facts | +| --- | --- | +| `manual` | accountable actor, Service, request ID | +| `jtype` | receipt/occurrence ID, Card ref, external actor, Automation owner | +| `scm` | verified receipt/delivery ID, provider actor, repository, object, action, ref/PR number | +| `cron` | occurrence ID, rule owner, scheduled timestamp, `[window_start, window_end)`, output mode | +| `api` | scoped service principal and idempotency key | + +Each adapter has different dedupe/concurrency semantics. They share resolver and +canonicalization code, not delivery defaults. A newer synchronized PR event uses +the new head SHA and may cancel an older queued/running Review for the same PR; +delivery replay of the same receipt is idempotent. + +| Kind | Idempotency key | Concurrency group | Cancellation/coalescing | +| --- | --- | --- | --- | +| `manual` | Service + request ID | none by default | never auto-cancel | +| `jtype` | Automation + occurrence/receipt ID | workflow + Card ID | Card leaving active states cancels queued/running work; terminal state prevents retry | +| `scm` | Automation + verified delivery ID + action | workflow + repository + PR/object ID | newer PR head cancels older Review; replay of one delivery is ignored | +| `cron` | Automation + UTC window start | Automation ID | one occurrence per window; overlapping tick is coalesced, not duplicated | +| `api` | scoped principal + Service + idempotency key | none unless Workflow defines one | no implicit cancellation | + +Cron `scheduled timestamp` and window boundaries are UTC RFC3339 instants with +second precision. The occurrence identity is derived from Automation ID plus the +exact `scheduled_for` instant; timezone conversion happens only when calculating +the schedule, never during dedupe. + +### 3.3 Run SCM Grant schema + +Ship-R1 deepens the existing `run_plugin_snapshots` aggregate instead of introducing +a second credential table. For the Service's SCM installation, the dispatch +transaction additionally freezes: + +| Field | Purpose | +| --- | --- | +| `repository_id` | provider-stable repository identity | +| `repository_path` | frozen owner/name used by provider operations | +| `clone_url` | frozen provider clone route; never includes credential | +| `default_branch` | frozen Service repository baseline | +| `acting_principal_kind` | installation/bot/app kind used for provider writes | +| `acting_principal_id` | provider-stable non-secret principal identifier | + +Provider/config/credential revisions already exist in the row. Together these +fields are exposed inside the control plane as `RunSCMGrant`. Public APIs return +only non-secret audit facts and revision identifiers. + +The dispatch transaction locks provider config, installation and repository +binding in stable order. A mismatch returns `dispatch_claim_unavailable`; it +does not build a partial snapshot. New Plugin-bound runs may not call the current +Service credential resolver after this point. + +Base/head revision pair remains on `runs` because it describes Review input, not +authorization. Create/preview checks are named `scm_install_ready`; +`run_scm_grant` does not exist until dispatch claim. If claim detects +reconnect/rename/config drift, the queued Run fails with +`dispatch_claim_unavailable`, stores no partial grant, and requires a new Run +resolved from current state. + +Grant-consumer matrix is part of Ship-R1 acceptance: + +| Operation | Frozen inputs | Forbidden after claim | +| --- | --- | --- | +| source bundle / short-lived clone credential | provider/config/credential revision、repository identity/clone route | `ResolveForService`、current Service repository/binding | +| source bundle push | same grant + delivered bundle identity | current Service credential/repository | +| PR create/update/readiness | same grant + typed `create_pull_request` output | current `RepoOwnerName`、current ready policy | +| Review PR read/commit check | same grant + Run revision pair | live repository binding or branch-only fallback | +| Review publish/summary fallback | same grant + validated result + plan hash | current Service credential/repository | + +Tests instrument every provider adapter call and assert the same installation, +provider/config/credential revision and repository identity across the row. A +new Plugin-bound Run has no compatibility resolver fallback. + +### 3.4 Review Plan schema v1 + +```json +{ + "schema_version": 1, + "plan_hash": "sha256:…", + "rules_revision": "review-v2", + "base_ref": "main", + "head_ref": "feature/x", + "base_sha": "40hex", + "head_sha": "40hex", + "merge_base_sha": "40hex", + "files": [ + { + "path": "src/app.ts", + "hunks": 2, + "changed_line_ranges": [{"start": 18, "end": 20}] + } + ], + "changed_files": 1, + "eligible_files": 1, + "eligible_hunks": 2, + "indexed_hunks": 2, + "eligible_changed_lines": 3, + "coverage_state": "complete", + "skipped": [] +} +``` + +The parser stores only paths, counts and right-side changed-line ranges. It never +persists the diff body. `files` means eligible/indexed text files; public API may +return their per-file counts but not ranges. `skipped` elements are +`{path, reason, detail}` and Ship-R1 reasons are `binary` or `unsupported`. + +Ship-R1 coverage is **input coverage**. `complete` means every eligible text hunk +entered the bounded review input; it does not claim the model semantically read +every hunk. Binary/unsupported files produce `partial`. A diff exceeding hard +byte/file/hunk limits fails with `review_input_too_large` before the model; it is +not represented as a misleading partial `limit` skip. `plan_hash` is SHA-256 over +the canonical plan with `plan_hash` omitted and is the equality key for identical +retry versus conflict. + +## 4. Dispatch and runtime sequence + +```mermaid +sequenceDiagram + participant Hook as GitHub webhook + participant API as Trigger adapter + participant Store as Store + participant Rec as Reconciler + participant Src as Source adapter + participant Run as Runner bootstrap + participant Agent as jcode Agent + participant GH as GitHub adapter + + Hook->>API: verified PR event (base/head SHA) + API->>API: resolve Workflow Contract + API->>Store: create queued Review Run + contract + Rec->>Store: dispatch claim + Run SCM Grant + Run->>Src: GET source with Run token + Src->>Store: load frozen SCM grant + Src->>Src: issue short-lived credential, bundle exact PR ref + Src-->>Run: credential-free bundle + Run->>Run: git diff baseSHA...headSHA + Run->>Store: first-write Review Plan (diff parsed, body discarded) + Run->>Agent: immutable review protocol + diff file + Agent->>Store: structured Review Result + Store->>Store: validate every anchor against Review Plan + Rec->>Store: load same frozen SCM grant + Rec->>GH: publish validated review +``` + +### 4.1 Exact commit behavior + +- current `NormalizedSCMEvent` has only `HeadSHA`; Ship-R1 explicitly adds + `BaseSHA` and persists it instead of assuming this already exists; +- GitHub reads `pull_request.base.sha`/`head.sha`; Gitea reads the equivalent PR + commit fields; GitLab reads `object_attributes.diff_refs.base_sha/head_sha`; +- provider fixtures must prove the exact payload path. A provider/action without + both values returns `review_revision_unavailable` rather than inventing one; +- an automatic Review without both SHAs is `blocked` before queue, not downgraded + to branch-only review; +- manual “Review this Cloud PR” Ship-R1 uses the source Run's pushed commit as head; + if an exact base SHA cannot be resolved by the provider adapter, it returns a + typed `review_revision_unavailable` conflict; +- source bundle construction checks that the fetched PR head equals frozen head + SHA. A changed ref is `review_source_drift`, never silently reviewed. +- Runner computes `merge_base_sha = git merge-base base_sha head_sha` and builds + the three-dot diff from that merge base. Run UI displays event base/head and + plan merge-base separately so reproduction does not conflate them. + +### 4.2 Review Plan bootstrap + +The Runner bootstrap computes the diff from the credential-free source bundle and +immediately calls a dedicated +internal endpoint before starting ACP. The endpoint is first-writer-wins and +accepts only a `kind=review` Run in scheduling/running state. It parses a bounded +unified diff, stores the compact plan, and discards the body. If this call fails, +the model never starts. + +The internal endpoint, not the Runner, parses the diff, canonicalizes the Plan and +computes `plan_hash` in one Store transaction. Competing bootstraps therefore use +one equality implementation: first insert is `201`, a byte-different diff that +canonicalizes to the same plan is `200`, and a different canonical plan is `409`. + +Before diff creation, Ship-R1 bootstrap hard-checks the minimal Review runtime +(`git`, `rg`, source bundle, exact revisions and internal API reachability). A +missing item fails as `review_runtime_unavailable` before the model. This is a +small fail-fast invariant for the shipped image, not the general profile-aware +Readiness evaluator planned for Ship-R2. + +### 4.3 Result validation + +`ReviewResult.ValidateAgainst(plan)` first performs current structural checks, +then requires every finding path to exist in the plan and every `line`/`end_line` +to fall inside right-side changed ranges. Duplicate anchors remain rejected. A +finding on a context or deleted line returns `invalid_review_anchor` and the +entire output is rejected, preserving the existing all-or-nothing contract. +For a rename, the plan indexes the new/right-side path; pure renames with no +changed right-side lines cannot receive inline findings and appear as skipped. + +## 5. API design + +### 5.1 Existing Run response additions + +`GET /api/v1/runs/{id}` returns: + +```json +{ + "execution_contract": {"schema_version": 1, "hash": "sha256:…"}, + "pr_base_sha": "…", + "pr_head_sha": "…", + "review_plan": { + "plan_hash": "sha256:…", + "rules_revision": "review-v2", + "base_sha": "…", + "head_sha": "…", + "merge_base_sha": "…", + "changed_files": 13, + "eligible_files": 12, + "changed_hunks": 37, + "indexed_hunks": 36, + "changed_lines": 204, + "coverage": "partial", + "files": [ + {"path": "src/app.ts", "status": "indexed", "hunks": 2, "changed_lines": 3}, + {"path": "logo.png", "status": "skipped", "reason": "binary", "hunks": 1, "changed_lines": 0} + ] + } +} +``` + +The public response includes per-file counts needed by Console but omits +individual changed-line ranges. An internal Store value retains ranges for +validation. Viewer receives the contract public projection (identity, profile +name/revision, trigger summary, execution/delivery/verification metadata and +hash); Project Member additionally receives bounded profile instructions. +Neither projection includes secret-bearing config, credential source, raw diff, +or encrypted data. + +### 5.2 Internal Review Plan endpoint + +`POST /internal/v1/runs/{id}/review-plan` + +- auth: existing Run token only; +- content type: `application/json`; +- body: `{ "base_sha": "…", "head_sha": "…", "merge_base_sha": "…", "diff": "…" }`; +- the Run owns the exact base/head pair; the Runner reports it back with the + workspace merge-base and bounded unified diff; +- `201`: first plan stored; +- `200`: identical retry whose computed canonical `plan_hash` matches storage; +- `409 review_plan_conflict`: a different plan already exists; +- `409 review_revision_mismatch`: workspace SHAs differ from Run snapshot; +- `400 invalid_review_plan`: shape, revision syntax, or diff parsing is invalid; +- `413 review_input_too_large`: hard byte/file/hunk limit exceeded. + +### 5.3 Future R2 endpoints reserved by design + +- `GET/POST /api/v1/projects/{id}/agent-profiles`; +- `GET/PATCH/DELETE /api/v1/agent-profiles/{id}`; +- `POST /api/v1/services/{id}/runs/preflight`; +- `GET /api/v1/runner-profiles` for Cluster Admin. + +Ship-R1 Console does not call or mock these endpoints. + +## 6. UI design + +Visual source of truth: + +- `design/workflow-contract.html` — honest Ship-R1 Composer contract + Run inspector; +- `design/review-coverage.html` — deterministic Review Run detail; +- shared assets under `design/assets/workflow-contract.*`. + +The direction is “flight plan / execution manifest”: warm paper background, +compact monospace identifiers, a single orange action accent, strict rows and +status stamps. It reuses the existing jcode Cloud tokens and avoids a node-canvas +metaphor because Ship-R1 is about verifiable facts, not arbitrary graph authoring. + +### 6.1 Composer / readiness anatomy + +Ship-R1 shows the built-in Developer profile as a read-only pill and a compact +“Execution contract” disclosure. Ship-R2 turns the pill into a profile selector and +adds live readiness checks. The prototype visibly labels R2-only controls as +planned; production Ship-R1 never presents a dead selector. + +Ship-R2 的 Automation 列表采用 OpenHands 已验证的最小信息集：name、trigger、enabled、 +profile/backend 与 last run；detail 再展示 prompt、repository/branch、timeout、allowed +outputs、revision 和 activity log。四个 proven workflow 从 conversation-assisted +setup 进入 typed draft；import 后默认 disabled，避免未经复核即触发外部写入。 + +Ship-R1 information order: + +1. Prompt; +2. Profile + permission + branch + model; +3. expanded contract facts, actual timeout and typed delivery; +4. Send. + +Ship-R2 inserts readiness status (`Ready`, `Blocked`, `Needs recheck`) and repair +action before Send. It is not rendered by Ship-R1 production code. + +### 6.2 Run contract inspector + +The inspector adds **Workflow contract** above technical execution facts: + +- Profile + revision; +- Workflow + revision + definition hash; +- Trigger; +- Model/effort/permission; +- actual effective timeout（Runner backend/profile appears in Ship-R2）; +- Delivery (`Ready PR`, `Session Draft → Ready`, `Review comment`, or `Diff only`); +- Verification; +- contract hash; +- SCM grant repository + provider/config/credential revision; +- legacy empty state. + +No secret, token source string, encrypted field or raw profile system text is +shown. Full instructions are visible only to Project members and contain no +authority-bearing data. + +### 6.3 Review coverage + +Review Run header shows base/head short SHA and rules revision. A coverage strip +shows files, hunks and changed lines presented to the reviewer. States: + +- **Complete input** — every eligible hunk was in the bounded review input; +- **Partial input** — skipped rows list reason (`binary`, `unsupported`); +- **Plan unavailable** — legacy Run; findings remain visible but are not labelled + deterministic; +- **Invalid output** — persistent failure with the rejected anchor reason. + +“No findings” is a result card, not a green coverage badge. Coverage and finding +count are separate facts. + +The fixture must label truncated per-file lists (`showing N of M`) and includes +visible examples for partial, plan-unavailable and zero-finding states; a single +green complete fixture is not the whole acceptance contract. + +### 6.4 Permission, tools and typed outputs + +| Contract fact | Runner meaning | Control-plane meaning | +| --- | --- | --- | +| `permission_mode=full_access` | no interactive approval inside the isolated Run; it does **not** grant provider credentials | none | +| `workspace_access=read_only` | Review may read source and create ephemeral diff/result files only; repository mutation/push is unavailable | Review output may still be published by adapter | +| `provider_credentials=none` | no provider token or Git remote credential enters Runner | adapter resolves only the frozen Run SCM Grant | +| `delivery.outputs[].type=provider_review` | Agent may emit the structured Review result only | adapter may publish that validated result to the triggering PR | +| `delivery.outputs[].type=create_pull_request` | implementation Run may emit a tested source bundle; Runner still cannot push with provider credentials | source/push/PR adapters may publish under the frozen grant | + +`delivery.outputs` is authoritative. There is no parallel mutable `mode`. PR-only +lifecycle fields exist only on `create_pull_request`; a `provider_review` contract +does not carry `ready_policy`. + +Legacy Run delivery fields may be projected for history, but a Ship-R1 contract +Run must not read a legacy delivery mode to choose behavior. If stored legacy +fields conflict with `delivery.outputs`, contract validation fails closed; it +does not select either branch dynamically. + +### 6.5 Pull Request lifecycle truth table + +| Session | Ready policy | Success | Failure/cancel | +| --- | --- | --- | --- | +| false | `lifecycle_aware` | create/update Ready PR after final bundle | no new PR; an existing managed Draft stays Draft with visible failure | +| true | `lifecycle_aware` | intermediate pushes may create/update Draft; final verified bundle marks Ready | managed PR remains Draft and links the terminal Run state | +| any | `always_draft` | explicit compatibility policy keeps Draft | remains Draft | + +Cloud never approves, merges, or explicitly dispatches repository CI. + +“Final verified bundle” is machine-checkable: the Run is terminal-success, all +declared required verification records are pass, the pushed commit equals the +final delivered bundle commit, and PR create/update is acknowledged in the +Delivery ledger. Only the control-plane lifecycle adapter can perform the +idempotent Draft → Ready transition. Any missing condition keeps Draft and emits +a visible blocker. + +### 6.6 Coverage/result truth table + +| Plan state | Result state | UI meaning | +| --- | --- | --- | +| `complete` | `valid_no_findings` | complete input, no validated finding; not a proof of bug-free code | +| `partial` | `valid_no_findings` | partial input, no finding in indexed input; never rendered green-complete | +| `complete` or `partial` | `valid_findings` | show coverage and finding count as independent facts | +| `unavailable` | any legacy result | legacy, not labelled deterministic | +| any | `invalid` | persistent Review failure with rejection code | + +Console never derives Plan state from finding count. + +### 6.7 Responsive and accessible behavior + +- ≥ 980px: transcript/content and 320px inspector columns; +- 700–979px: inspector below main content, summary facts in two columns; +- < 700px: one column, coverage metrics remain three compact cells; +- disclosure controls are real buttons with `aria-expanded`; +- status never relies on color alone; +- hashes use selectable text and wrap safely; +- loading preserves the panel frame; error preserves last-known contract. + +## 7. Security and privacy + +1. Contract/profile text cannot override platform Delivery/Security sections; +2. Run SCM Grant public projection contains identifiers only, never token or + encrypted bytes; +3. source and writeback resolve credentials exclusively from frozen versions; +4. raw webhook payload and raw diff are discarded after normalization/parsing; +5. review plan first-write prevents agent-time replacement; +6. changed path validation rejects absolute paths, traversal and backslashes; +7. Git error redaction remains mandatory for every grant consumer; +8. legacy credential fallback is disallowed for a new Plugin-bound Run. +9. external writes must match `delivery.outputs`; the Agent cannot add + MCP/provider mutations outside the contract. + +## 8. Failure model + +| Code | Phase | User message | Repair role | +| --- | --- | --- | --- | +| `workflow_contract_invalid` | create | Workflow contract could not be resolved | Cloud operator | +| `delivery_output_conflict` | create/delivery | Contract output conflicts with legacy delivery state | Cloud operator | +| `dispatch_claim_unavailable` | queue | Plugin or repository changed before launch | Project Owner | +| `review_revision_unavailable` | create | Exact review commits are unavailable | Repository maintainer | +| `review_source_drift` | source | PR head no longer matches the accepted event | Re-run from latest PR event | +| `review_runtime_unavailable` | bootstrap | Required Review runtime is unavailable | Cluster operator | +| `invalid_review_diff` | bootstrap | Review input could not be indexed | Cloud operator | +| `review_input_too_large` | bootstrap | Review input exceeded deterministic limits | Automation owner | +| `review_plan_conflict` | bootstrap | Review plan changed after it was frozen | Cloud operator | +| `invalid_review_anchor` | result | Review output referenced a non-changed line | Retry review | + +None of these falls back to a different repository, credential, branch or output. + +## 9. Migration and compatibility + +- existing Runs keep `{}` contract and NULL review plan; UI says “Created before + workflow snapshots”; +- old Runner images may submit structured Review without plan during a bounded + rollout window only when the Run contract is legacy; Ship-R1 contract Runs require + a plan; +- new Orchestrator may launch old image only if version gate marks it incompatible + and blocks new deterministic Review, avoiding mixed semantics; +- existing `always_draft` services retain their policy; new lifecycle-aware + behavior is unchanged; +- schema rollback is safe because all columns are additive; application rollback + ignores them. + +## 10. Test matrix + +### Domain + +- contract canonical hash and tamper detection; +- built-in profile mapping by Run kind; +- unified diff: added/context/deleted/rename/binary/multiple hunks/no-newline; +- range compression and maximum bounds; +- finding anchor validation. + +### Store + +- Memory/PostgreSQL contract and review plan round-trip; +- plan first-write identical retry vs conflict; +- dispatch claim atomically freezes repository + credential/config revisions; +- reconnect/rename after claim does not change snapshot; +- failure rolls back the entire claim. + +### API/runtime + +- all Run creation paths obtain contract; +- contract includes stable workflow identity/revision and timeout; +- internal review-plan auth/state/content/size checks; +- exact SHA mismatch fails before Agent turn; +- fixture with event base != computed merge-base preserves all three fields and + builds anchors from `merge_base_sha...head_sha`; +- source uses frozen grant after Service mutation; +- review writeback uses the same grant; +- delivery rejects output types absent from `delivery.outputs`; +- legacy compatibility is explicit. + +### Console + +- contract facts render with exact labels; +- legacy empty state; +- complete/partial review coverage; +- no-finding and coverage remain separate; +- keyboard disclosure and narrow widths. + +## 11. Implementation order and commit boundaries + +1. `docs(product): define workflow contract and review design`; +2. `feat(orchestrator): freeze workflow and SCM execution contracts`; +3. `feat(review): validate deterministic review plans`; +4. `feat(console): expose workflow contract and review coverage`; +5. `test(runner): prove deterministic review bootstrap`; +6. audit fixes, full tests, Ready PR, main merge and deployment. diff --git a/docs/30-open-source-agent-workflow-benchmark.md b/docs/30-open-source-agent-workflow-benchmark.md new file mode 100644 index 00000000..b2e57406 --- /dev/null +++ b/docs/30-open-source-agent-workflow-benchmark.md @@ -0,0 +1,211 @@ +# 30 · 开源 Agent Workflow 产品对标 + +调研日期：2026-08-01 +用途：约束 `docs/28-workflow-contract-and-deterministic-review-prd.md` 与 +`docs/29-workflow-contract-and-review-design.md` 的产品、架构和 UI 决策。 + +## 1. 结论 + +**主产品对标选择 [OpenHands Agent Canvas](https://github.com/OpenHands/OpenHands)。** +它在调研时有 82,724 stars、MIT license，已经把“对话、Agent Profile、LLM +Profile、Backend、Automation、Run history”组织成一个自托管控制台，并直接提供 +PR Review、Repository Monitor、schedule/webhook trigger 与 Kubernetes backend。 +它与 jcode Cloud 的产品边界最接近，也足够高星。 + +但 jcode 不应复制任何一个项目的全部实现。最终参考组合是： + +| 层 | 主要参考 | 借鉴点 | +| --- | --- | --- | +| 产品与 UI | OpenHands Agent Canvas | Profile/LLM/Backend 分层，Automate 控制台，proven workflow，会话式配置，运行历史 | +| Workflow authoring 与安全 | GitHub Agentic Workflows (`gh-aw`) | Markdown + frontmatter、编译产物、只读默认、typed safe outputs、权限/网络/供应链 guardrails | +| JType pickup 与长任务运行 | OpenAI Symphony | tracker polling、单一调度权威、per-work-item workspace、reconcile、retry、continuation | +| PR Review | Alibaba OpenCodeReview | 确定性文件选择/分组/规则/定位 + Agent 判断，覆盖统计，精确行评论与幂等发布 | + +所以 jcode 的定位不是另一个通用 DAG engine，而是： + +> 面向软件工作的多后端 Agent control plane：把来自 Conversation、JType、SCM、 +> Cron 与 API 的请求编译为不可变 Workflow Contract，在具备能力证明的 Runner +> 上执行，并由控制面安全交付 Review、PR、Card、Report 等结构化结果。 + +## 2. 样本与活跃度快照 + +星数来自 GitHub repository API，仅作为 2026-08-01 的活跃度快照，不是质量评分。 + +| 项目 | Stars | Forks | License | 与 jcode 的距离 | +| --- | ---: | ---: | --- | --- | +| [OpenHands/OpenHands](https://github.com/OpenHands/OpenHands) | 82,724 | 10,637 | MIT | 最接近完整产品：自托管控制台 + 多 Agent backend + Automation | +| [openai/symphony](https://github.com/openai/symphony) | 26,356 | 2,667 | Apache-2.0 | 最接近 JType/看板任务持续 pickup | +| [alibaba/open-code-review](https://github.com/alibaba/open-code-review) | 17,036 | 1,158 | Apache-2.0 | 最接近确定性 PR Review 子系统 | +| [github/gh-aw](https://github.com/github/gh-aw) | 4,841 | 473 | MIT | 最成熟的 repository-owned Workflow contract 与 safe output 参考 | + +`gh-aw` 的星数不是四者最高，但它是 GitHub 官方、功能距离最近的 Workflow +authoring/security 参考；OpenHands 则满足“高星主对标”的要求。 + +## 3. OpenHands Agent Canvas：主产品对标 + +### 3.1 它怎么做 + +- Canvas 只负责 UI、backend/profile selection 和 API adapter；Agent Server 负责 + conversation，Automation Server 负责 schedule/event run，sandbox/workspace + 不由前端承担。 +- Agent Profile 与 LLM Profile 是两个对象。前者选择 OpenHands 或 ACP Agent + （Claude Code、Codex、Gemini、自定义 ACP），后者只管理 provider/model/credential。 +- Automation 有 schedule 或 event trigger、prompt、repository、branch、model、 + plugin、notification、timeout、enabled 与 run history。 +- UI 提供 proven workflow。用户从 PR Review 或 Repository Monitor 模板进入一段 + setup conversation，由 Agent 追问 repository、event、output、CI 和 ignore rule， + 然后创建 Automation。 +- Automation 可以导出为 versioned JSON；导入时校验 schema，并默认 disabled， + 由用户复核后启用。 +- 当前前端类型把默认 timeout 描述为 10 分钟、服务端上限 30 分钟，正好覆盖 + jcode 首批需要持续至少三十分钟的 POC 场景。 + +### 3.2 借鉴 + +1. Console 分开呈现 **Agent Profile、LLM Profile、Runner Backend**，避免角色、模型 + 和执行环境混成一个下拉框。 +2. 首批提供 proven workflows，而不是先做任意 DAG： + - GitHub PR Review； + - JType Developer pickup； + - Competitor research → JType Card； + - Weekly repository report/changelog。 +3. Automation detail 至少展示 prompt、trigger、profile、repository/branch、timeout、 + enabled、最近 Run 和 activity log。 +4. 配置可以从 Conversation 产生，但 publish/enable 前必须变成 typed definition， + 经过 preview 与 readiness，而不是直接把整段对话当运行合同。 +5. Import 默认 disabled；profile/workflow edit 只影响未来 Run，历史 Run 固定旧 revision。 + +### 3.3 不照搬 + +OpenHands 的 PR Review 快速路径让 Agent 通过 GitHub MCP 使用具备 Contents、Issues、 +Pull Requests 写权限的 token。这个方式部署简单，但 Agent、prompt injection 与写权限 +在同一信任域，且文档中的最小权限仍比纯 Review 所需更宽。 + +jcode 保持更强边界：Runner/Agent 只拿 source bundle 和 read-only/allowlisted tools； +Agent 输出结构化 intent；Run SCM Grant 留在 control plane，由 provider adapter 校验 +并发布评论或 PR。这样 Agent Profile、MCP 与 credential selection 彼此独立。 + +## 4. GitHub Agentic Workflows：Workflow 与安全参考 + +### 4.1 它怎么做 + +- 用户维护 Markdown 文件：YAML frontmatter 定义 trigger、permission、engine、MCP、 + safe outputs 与预算，正文是自然语言任务。 +- `gh aw compile` 把可读源文件编译成 `.lock.yml`；两者都提交到 repository。 +- trigger 覆盖 manual、issue/comment、PR/review、push 与人类友好的 schedule；编译器 + 把 fuzzy schedule 转成确定 cron，并为不同 workflow 分散执行时间。 +- Agent 默认只读。创建 issue/comment/PR 等写操作不是 Agent 直接执行，而是 buffered + typed safe output，经过数量/格式/secret/threat policy 后才由独立阶段落地。 +- 编译期做 schema validation、expression allowlist、action SHA pinning 与 security + scan；运行时再做 sandbox、network firewall、tool allowlist、rate limit 和 approval。 +- PR 触发按 PR/ref 做 concurrency group，新 commit 可取消旧 review run。 + +### 4.2 借鉴 + +1. 将 **Workflow Definition** 与 **Execution Contract** 分开：前者可读、可编辑、 + 可版本控制；后者是每次 Run 的 compiled immutable snapshot。 +2. R2 支持 repository-owned `.jcode/workflows/*.md`：frontmatter 管 trigger/profile/ + requirements/timeout/output，正文管理 prompt；Cloud UI 编辑同一 schema，不另造语义。 +3. Delivery 使用 allowlisted typed outputs，例如 `provider_review`、`create_pull_request`、 + `create_jtype_card`、`update_changelog`、`publish_report`。 +4. `enable` 等价于 publish：只有 schema、capability、permission 和 output policy 都通过 + 才能生效。配置修改产生新 revision，不热改 in-flight Run。 +5. 受信 compiler/resolver 决定阶段和权限；Agent 只能建议 intent，不能扩大 contract。 + +### 4.3 不照搬 + +`gh-aw` 绑定 GitHub Actions。jcode 需要支持 GitHub/GitLab/Gitea、JType、持久 workspace、 +长 session 与 Kubernetes Runner，因此不能把 `.lock.yml` 或 Actions job 当核心运行模型。 +Cloud Store 中的 Workflow Contract 才是跨 provider 的权威；source-controlled Markdown +只是一个 authoring adapter。 + +## 5. OpenAI Symphony：JType pickup 与恢复参考 + +### 5.1 它怎么做 + +Symphony 是长期运行的 tracker reader/orchestrator。它从 repository-owned +`WORKFLOW.md` 读取 YAML frontmatter 和 prompt，轮询 Linear，确定 eligibility，按全局和 +state concurrency 派发，在每个 issue 的隔离 workspace 中启动 Agent。每个 tick 先 +reconcile 再 dispatch；issue 进入非 active/terminal 状态会停止 worker，失败按指数 +退避重试，正常 turn 后可以在同一 thread/workspace 继续。 + +它强调：Orchestrator 是 claim/running/retry 的单一写入权威；workspace path 必须可预测 +且限制在 root；hook 有 timeout；配置 reload 只影响未来 dispatch，不要求中断 in-flight +session。其参考实现重启后依赖 tracker + filesystem 恢复，没有 durable scheduler DB。 + +### 5.2 借鉴 + +1. JType adapter 只负责 receipt/claim/eligibility，Run Store 是 dispatch/retry/reconcile + 的单一权威。 +2. 同一 Work Item 的 continuation 保持同一 workspace 与 session lineage；Card 状态变化 + 会取消或停止不再 eligible 的 Run。 +3. 每个 trigger tick 先 reconcile，再 dispatch；必须有 stall timeout、retry backoff、 + global/profile/runner concurrency。 +4. Workflow revision reload 只影响未来 Run；in-flight Run 继续消费 frozen contract。 +5. jcode 已有 PostgreSQL，不接受 Symphony “重启不保留 retry/session”这一预览级限制； + retry、occurrence、checkpoint 与 delivery ledger 必须持久化。 + +## 6. Alibaba OpenCodeReview：Review 参考 + +### 6.1 它怎么做 + +OpenCodeReview 明确反对仅靠通用 Agent prompt 做 Review。工程层确定文件筛选、相关文件 +分组、规则匹配、coverage、评论定位和二次 reflection；Agent 负责动态判断与按需读取 +上下文。大 diff 被拆成隔离 review units，可并发执行。 + +输出包含 path、start/end line、category、severity、existing/suggestion code；GitHub +publisher 固定 PR head commit、按路径/行发布 inline comment，对无有效行号或策略降级的 +finding 放到 summary。发布端还有稳定排序、批次上限、run/review/comment marker 与重试 +reconciliation，避免 provider 5xx 后重复评论。 + +### 6.2 借鉴 + +1. Review Plan 必须在模型前完成 file/hunk selection、skip reason 与 changed-line index。 +2. finding 只能命中本次 diff 的 right-side changed line，且带 severity/category。 +3. `0 finding` 不代表 coverage complete；coverage 与结果数量分开显示。 +4. 发布固定 head SHA，并使用 deterministic ordering、bounded batch 与 idempotency marker。 +5. R4 的多 Agent 仅用于 plan 产生的互斥 review units；每个 shard 有隔离 budget，最终由 + deterministic reducer 去重/反思，不开放 Agent 自主递归委派。 + +## 7. 对四个目标场景的落地 + +| 场景 | Trigger adapter | Profile | Required capabilities | Typed outputs | 关键完成条件 | +| --- | --- | --- | --- | --- | --- | +| PR Review | SCM verified event | Reviewer | source.read、git、scm.review.write | provider_review | fixed SHA、coverage、valid anchors、idempotent publish | +| 竞品研究 → JType | Manual/Cron | Product Manager | web/MCP fetch、jtype.create-card | create_jtype_card、publish_report | sources、request schema、Card receipt | +| JType request → PR | JType receipt/claim | Developer | source.read/write、toolchain、scm.pr.write | create_pull_request | tests、commit、Ready PR、Card backlink | +| 周报/Changelog | Cron occurrence | Developer 或 PM | source.read、scm.read、optional write | publish_report/update_changelog | fixed time window、dedupe、report/PR receipt | + +四者不需要四套 engine。它们需要不同的 trigger verification、idempotency key 和 +writeback adapter，但共享 Workflow Definition → Compile/Resolve → Readiness → Immutable +Run Contract → Runner → Validated Output → Delivery Ledger。 + +## 8. 因对标而改变的设计决定 + +1. 在 Workflow Contract 中新增 `workflow.id/revision/source/definition_hash`，不再只靠 + profile 和 trigger 推断“执行的是哪个 workflow”。 +2. `execution.timeout_seconds` 成为合同字段；Ship-R1 冻结与实际 + `activeDeadlineSeconds` 相同的唯一 effective value，POC 对该值要求不低于 1800 + 秒而不引入第二个默认值；Ship-R2 才由 + Runner profile 上限和 readiness 阻塞不兼容请求。 +3. R2 增加 portable Workflow Definition；UI 和 repository file 使用同一 versioned + schema，导入默认 disabled。 +4. Delivery Contract 改为 allowed typed outputs；Draft/Ready 是 PR output 的 lifecycle + policy，不是独立 workflow。 +5. Automation/Profile edit 只影响 future Run；Run detail 分开显示 workflow/profile + revision、LLM selection、effective timeout/未来 Runner digest 与 SCM grant revision， + 不伪造 Ship-R1 尚不存在的 LLM/Runner Profile revision。 +6. Ship-R2 UI 提供四个 proven workflows 与 conversation-assisted setup；高级 DAG 继续不在 + 范围内。 +7. Review publisher 的幂等与批次 proof 纳入 R1 测试，而不是只验证 JSON 形状。 + +## 9. 参考资料 + +- [OpenHands repository 与 Agent Canvas architecture](https://github.com/OpenHands/OpenHands) +- [OpenHands Automations](https://docs.openhands.dev/openhands/usage/agent-canvas/managing-automations) +- [OpenHands Agent Profiles](https://docs.openhands.dev/openhands/usage/agent-canvas/agent-profiles) +- [OpenHands PR Review Assistant](https://docs.openhands.dev/openhands/usage/agent-canvas/prebuilt/github-pr-review) +- [GitHub Agentic Workflows: How They Work](https://github.github.com/gh-aw/introduction/how-they-work/) +- [GitHub Agentic Workflows: Security Architecture](https://github.github.com/gh-aw/introduction/architecture/) +- [OpenAI Symphony specification](https://github.com/openai/symphony/blob/main/SPEC.md) +- [Alibaba OpenCodeReview](https://github.com/alibaba/open-code-review) diff --git a/docs/31-workflow-case-conversations-and-video-agent-poc.md b/docs/31-workflow-case-conversations-and-video-agent-poc.md new file mode 100644 index 00000000..72edefeb --- /dev/null +++ b/docs/31-workflow-case-conversations-and-video-agent-poc.md @@ -0,0 +1,101 @@ +# 31 · Workflow case conversations 与 Video Prompt Studio POC + +状态：Ship-R1 验收用例 / Ship-R2 输入 +日期：2026-08-01 + +## 结论 + +不同场景需要不同的 trigger adapter、幂等键、输入事实和外部回写，但不需要复制 +四套执行引擎。它们都应收敛为同一种不可变 `WorkflowContract`，再由一个 Runner +执行。Agent 知道自己应当做什么，不是靠猜对话，而是因为 Cloud 在创建 Run 时冻结 +了 workflow、profile、模型、权限、timeout、delivery、verification 和 requirements。 + +`Developer / PM / Architect / Reviewer` 首先是 Profile，不等同于多 Agent。只有任务能 +安全切成互不重叠的 work unit，并且存在预算、隔离、合并规则和 loop limit 时，才应 +fan-out。普通实现、研究或架构任务使用一个明确角色的 Agent，更容易解释和追责。 + +## End-user conversations + +每个 case 的有效执行 timeout 不低于 1800 秒。这里的“30 分钟”是可运行预算，不要求 +Agent 人为等待；提前满足验收条件可以提前完成。 + +| Case | 用户会怎么说 | Trigger / Profile | 系统应完成什么 | 建议预算 | +| --- | --- | --- | --- | --- | +| PR Review | “检查这个 PR 的并发和权限问题，只报确定的问题。” | SCM PR / Reviewer | 冻结 base/head SHA，建立 changed-line plan，发布可定位 finding 与 coverage；不改代码 | 30–45m | +| 竞品研究入板 | “抓取这些竞品最近的 Video Agent 能力，整理成可执行 request 放进 JType。” | Manual/API / PM | 用 allowlisted MCP fetch，保留来源，去重并写入 JType；输出 research report 和 Card IDs | 45–90m | +| JType pickup | “把 Ready 列里的下一张卡实现掉。” | JType occurrence / Developer | 原子领取，冻结 Card receipt 和仓库授权，实现、测试、创建 Ready PR，回写 Run/PR 链接并移动卡片 | 30–120m | +| Weekly repo care | “每周总结最近改动，处理安全的小任务并更新 changelog。” | Cron / Developer | 固定时间窗口和 last-success cursor；生成 report；只有满足变更政策才提交 PR；幂等回写 changelog | 30–60m | +| Architecture proposal | “根据现有 ADR 给出队列恢复方案，不要直接改生产代码。” | Manual / Architect | 读取 CONTEXT/ADR，输出 alternatives、trade-off、failure model 和 ADR 草稿；delivery 为 document/diff-only | 45–90m | +| Video product dogfood | “做一个 MiniMax 视频 prompt 管理库，并能发起视频生成。” | JType/Manual / Developer | 从空仓库建立产品、测试 provider adapter、创建 Ready PR；API key 未配置时提供 mock mode，不伪造真实视频 | 60–180m | + +### PR Review 对话 + +用户只期待三件事：审的是当前这次改动、评论落在正确的行、没有问题时能区分“真的 +检查完”与“输入没覆盖完”。因此 Conversation 顶部应先显示 `base → head`、rules +revision 和 coverage，结论其次。Review workflow 永远是 read-only，不应因为 Service +之后改成可写就打开 PR。 + +### PM research → JType 对话 + +PM 期待的是带出处的 decision-ready request，而不是浏览摘要。Cloud 需要先做 +readiness：MCP fetch 与 JType write capability 缺一项就不入队；写 Card 前用 +`source URL + normalized title + board` 产生幂等键；结果同时保留来源、产品差距、 +建议、验收条件与新建 Card ID。该 case 不需要多 Agent；当来源很多时可做受控 research +shards，主 Agent 只合并结构化 evidence。 + +### JType pickup 对话 + +用户期待“卡片被谁领取、现在在哪一步、最终 PR 是什么”清楚可见。Cloud 先创建 +occurrence receipt，再冻结 Developer contract 和 SCM grant；失败时 Card 保持可修复 +状态，重复 delivery 不产生第二个 Run。成功默认创建 Ready PR；Draft 只用于长 session +尚未结束或 Owner 显式保留的历史策略。 + +### Cron 对话 + +Cron 的关键差异不是 prompt，而是窗口和重放。每次 Run 要冻结 scheduled time、 +`since/until`、上次成功 cursor 和 coalesce key。无安全变更时只发布 report；有改动时 +仍走相同 PR delivery，绝不直接 merge。Changelog 更新必须由测试与 diff policy 验证。 + +## Video Prompt Studio 产品 POC + +### API 名称校正 + +“MiniMax H3”按当前官方 API 应实现为 `MiniMax-Hailuo-2.3`，不是硬编码一个 `H3` +模型名。正式 Video Generation API 是异步三段式：创建任务、查询任务、取得文件。 +旧的 `/v1/video_template_generation` Video Agent 模板接口在官方 reference 已标记 +deprecated，因此产品首版不把它作为核心依赖，只保留未来 provider compatibility seam。 + +### 用户价值 + +团队可以把经过验证的镜头 prompt 当作可版本化资产，而不是散落在聊天记录中；可以 +用变量生成最终 prompt，选择 Text-to-Video 或 Image-to-Video，并在同一处查看异步任务 +状态、输入版本和结果。API key 只保留在服务端环境变量 `MINIMAX_API_KEY`，前端和日志 +不得显示。 + +### MVP 范围 + +- Prompt library：title、description、tags、mode、prompt template、variables、version； +- Prompt compiler：变量 schema、必填校验、2000 字符限制、camera command preview； +- Generation：`MiniMax-Hailuo-2.3`，T2V/I2V，6/10 秒与 768P/1080P 合法组合校验； +- Job ledger：local ID、provider task ID、prompt version、status、error、result URL、timestamps； +- Provider interface：真实 MiniMax adapter + deterministic fake adapter； +- 无 key 时的 mock mode 和显式 banner；绝不声称 mock result 来自 MiniMax； +- 单元测试、provider contract test、README、本地 compose；不实现计费、多人审批或素材库。 + +### Cloud dogfood 验收 + +1. 新建独立 GitHub repository； +2. 在 Cloud 建立 Service，Git delivery 使用 lifecycle-aware Ready PR； +3. 用 60–180 分钟 Developer Run 完成 tracer-bullet； +4. Run detail 能看到冻结 contract、模型、timeout、SCM grant 和最终 PR； +5. 没有 `MINIMAX_API_KEY` 时 fake provider 的 create→poll→success 流程通过； +6. 有 key 时才允许显式 opt-in 的 smoke generation，避免测试意外产生费用； +7. PR Review workflow 对生成的 PR 再跑一次确定性 Review； +8. CI 通过后合并；Cloud 不自动 approve 或 merge。 + +## Ship-R1 与 Ship-R2 边界 + +Ship-R1 提供不可变合同、Developer/Reviewer built-in Profile、SCM grant、确定性 Review +和 inspector，因此可以安全观察上述 case。PM/Architect/Custom Profile、MCP readiness、 +repository-owned workflow authoring、Video provider capability 和多 Agent shards 属于 +Ship-R2。当前 UI 不应提前展示一个实际上不能配置的 Profile selector。 diff --git a/orchestrator/cmd/orchestrator/main.go b/orchestrator/cmd/orchestrator/main.go index f3c277b4..10f785b4 100644 --- a/orchestrator/cmd/orchestrator/main.go +++ b/orchestrator/cmd/orchestrator/main.go @@ -66,6 +66,7 @@ func run(log *slog.Logger) error { if err := store.Migrate(ctx, st.Pool()); err != nil { return err } + store.ConfigureWorkflowTimeoutDefaults(st, cfg.RunTimeoutSecs, cfg.SessionTTLSecs) log.Info("migrations applied") if *migrateOnly { diff --git a/orchestrator/internal/api/api.go b/orchestrator/internal/api/api.go index a3e3c056..13f50a79 100644 --- a/orchestrator/internal/api/api.go +++ b/orchestrator/internal/api/api.go @@ -637,6 +637,7 @@ func (s *Server) Handler() http.Handler { // Review output may be stored as a run artifact. SCM clone/push/comment // operations are deliberately absent here: tasks use mounted Skills/CLIs. mux.Handle("POST /internal/v1/runs/{id}/review", s.runToken(s.handleIngestReview)) + mux.Handle("POST /internal/v1/runs/{id}/review-plan", s.runToken(s.handleIngestReviewPlan)) // Multi-turn session (D22): the runner's acpdrive reports each turn's // completion and long-polls for the next user message. RUN_TOKEN authed. mux.Handle("POST /internal/v1/runs/{id}/turn-complete", s.runToken(s.handleTurnComplete)) diff --git a/orchestrator/internal/api/api_test.go b/orchestrator/internal/api/api_test.go index 6bfc36bd..02ed4b74 100644 --- a/orchestrator/internal/api/api_test.go +++ b/orchestrator/internal/api/api_test.go @@ -351,6 +351,7 @@ func TestRetryPreservesReviewIdentity(t *testing.T) { Prompt: "AI review of PR x", Status: domain.StatusQueued, Kind: domain.RunKindReview, Phase: "Queued", Attempt: 1, PRHeadBranch: "jcode/run-abc12345", PRBaseBranch: "main", + PRHeadSHA: strings.Repeat("a", 40), PRBaseSHA: strings.Repeat("b", 40), CreatedAt: time.Now().UTC(), } if err := st.CreateRun(ctx, rev); err != nil { diff --git a/orchestrator/internal/api/automation_test.go b/orchestrator/internal/api/automation_test.go index 79f25359..802c6805 100644 --- a/orchestrator/internal/api/automation_test.go +++ b/orchestrator/internal/api/automation_test.go @@ -164,8 +164,8 @@ func TestGiteaPREventDispatchesOneReviewRunPerHeadSHA(t *testing.T) { "repository": map[string]any{"full_name": "acme/repo"}, "pull_request": map[string]any{ "html_url": "http://gitea.test/acme/repo/pulls/17", "draft": false, - "head": map[string]any{"ref": "feature", "sha": "abc123"}, - "base": map[string]any{"ref": "main"}, + "head": map[string]any{"ref": "feature", "sha": strings.Repeat("a", 40)}, + "base": map[string]any{"ref": "main", "sha": strings.Repeat("b", 40)}, }, } for i := 0; i < 2; i++ { @@ -209,7 +209,7 @@ func TestParseGiteaReviewEvent(t *testing.T) { payload := map[string]any{ "action": tt.action, "number": 1, "changes": tt.changes, "repository": map[string]any{"full_name": "acme/repo"}, - "pull_request": map[string]any{"html_url": "http://gitea/pr/1", "draft": false, "head": map[string]any{"ref": "x", "sha": "sha"}, "base": map[string]any{"ref": "main"}}, + "pull_request": map[string]any{"html_url": "http://gitea/pr/1", "draft": false, "head": map[string]any{"ref": "x", "sha": strings.Repeat("a", 40)}, "base": map[string]any{"ref": "main", "sha": strings.Repeat("b", 40)}}, } body, _ := json.Marshal(payload) got, _ := parseGiteaReviewEvent(tt.header, body) diff --git a/orchestrator/internal/api/plugin_webhook.go b/orchestrator/internal/api/plugin_webhook.go index 634f7514..b2ea6b76 100644 --- a/orchestrator/internal/api/plugin_webhook.go +++ b/orchestrator/internal/api/plugin_webhook.go @@ -350,7 +350,7 @@ func (s *Server) handlePluginWebhook(w http.ResponseWriter, r *http.Request) { userID := identity.UserID manualUserID = &userID pr, prErr := manualClient.PRByNumber(r.Context(), manualOwner, manualRepo, int(event.Object.Number)) - if prErr != nil || pr == nil || pr.HeadRef == "" || pr.BaseRef == "" { + if prErr != nil || pr == nil || pr.HeadRef == "" || pr.BaseRef == "" || pr.HeadSHA == "" || pr.BaseSHA == "" { manualReply("jcode couldn't read this pull request. Check that the App still has Pull requests: read and write permission.") if !recordDecision(a, svc, domain.AutomationExecutionBlocked, "pull_request_unavailable", "The repository Provider could not read this pull request.", "project_owner") { @@ -358,9 +358,39 @@ func (s *Server) handlePluginWebhook(w http.ResponseWriter, r *http.Request) { } continue } - event.Ref, event.BaseRef = pr.HeadRef, pr.BaseRef + event.Ref, event.BaseRef, event.HeadSHA, event.BaseSHA = pr.HeadRef, pr.BaseRef, pr.HeadSHA, pr.BaseSHA event.Object.URL = pr.URL } + // GitLab MR webhooks commonly omit diff_refs.base_sha. Keep the queue + // fail-closed, but enrich an incomplete automatic Review from the current + // repository grant before resolving its immutable Run contract. Comment + // reviews already took this path above for authorization and acknowledgement. + if a.RunKind == domain.RunKindReview && event.Family == scmevent.FamilyPullRequest && + (event.Ref == "" || event.BaseRef == "" || event.HeadSHA == "" || event.BaseSHA == "") { + client, clientErr := s.pluginProviderClient(r.Context(), binding) + owner, repo, splitOK := gitprovider.SplitRepo(binding.RepositoryPath) + if clientErr != nil || client == nil || !splitOK { + s.recordPluginAutomationError(r, a, "The repository Provider credential is unavailable for review revision lookup.") + if !recordDecision(a, svc, domain.AutomationExecutionBlocked, + "provider_credential_unavailable", "The repository Provider credential is unavailable.", "project_owner") { + return + } + continue + } + pr, prErr := client.PRByNumber(r.Context(), owner, repo, int(event.Object.Number)) + if prErr != nil || pr == nil || pr.HeadRef == "" || pr.BaseRef == "" || pr.HeadSHA == "" || pr.BaseSHA == "" { + s.recordPluginAutomationError(r, a, "The pull request revision pair could not be read from the Provider.") + if !recordDecision(a, svc, domain.AutomationExecutionBlocked, + "review_revision_unavailable", "The repository Provider could not supply the pull request base and head revisions.", "provider") { + return + } + continue + } + event.Ref, event.BaseRef, event.HeadSHA, event.BaseSHA = pr.HeadRef, pr.BaseRef, pr.HeadSHA, pr.BaseSHA + if pr.URL != "" { + event.Object.URL = pr.URL + } + } sel, outcome, selectErr := s.models.SelectModel(r.Context(), svc.ProjectID, deref(svc.DefaultModelID), a.ModelID) if selectErr != nil || outcome != modelcfg.SelectOK { s.recordPluginAutomationError(r, a, pluginAutomationModelError(outcome, selectErr)) @@ -439,6 +469,20 @@ func (s *Server) handlePluginWebhook(w http.ResponseWriter, r *http.Request) { if event.Family == scmevent.FamilyPullRequest || manualReview { run.PRHeadBranch = automationRunBranch(event) run.PRBaseBranch = strings.TrimPrefix(event.BaseRef, "refs/heads/") + run.PRHeadSHA = event.HeadSHA + run.PRBaseSHA = event.BaseSHA + } + if run.Kind == domain.RunKindReview && + (run.PRHeadBranch == "" || run.PRBaseBranch == "" || run.PRHeadSHA == "" || run.PRBaseSHA == "") { + s.recordPluginAutomationError(r, a, "The pull request revision pair is unavailable; the review was not queued.") + if manualReview { + manualReply("jcode couldn't freeze the pull request revision pair, so no review was queued. Refresh the pull request and try again.") + } + if !recordDecision(a, svc, domain.AutomationExecutionBlocked, + "review_revision_unavailable", "The pull request base and head revisions are required before queueing a review.", "provider") { + return + } + continue } if event.Object.URL != "" { run.PRURL = event.Object.URL diff --git a/orchestrator/internal/api/plugin_webhook_test.go b/orchestrator/internal/api/plugin_webhook_test.go index 960eec59..ddafe188 100644 --- a/orchestrator/internal/api/plugin_webhook_test.go +++ b/orchestrator/internal/api/plugin_webhook_test.go @@ -354,17 +354,17 @@ func TestPluginReviewAutomationCreatesReviewRunAndSkipsDrafts(t *testing.T) { "pull_request": map[string]any{ "id": 7, "number": 7, "draft": draft, "html_url": "https://gitea.example/acme/repo/pulls/7", "head": map[string]any{"ref": "feature", "sha": sha}, - "base": map[string]any{"ref": "main"}, + "base": map[string]any{"ref": "main", "sha": strings.Repeat("b", 40)}, }, } } - draft := f.postGitea(t, "pull_request", "draft-review", payload(true, "sha-draft")) + draft := f.postGitea(t, "pull_request", "draft-review", payload(true, strings.Repeat("c", 40))) draft.Body.Close() runs, _ := f.st.ListRunsByService(context.Background(), f.serviceID, 10) if len(runs) != 0 { t.Fatalf("draft review created %d runs", len(runs)) } - ready := f.postGitea(t, "pull_request", "ready-review", payload(false, "sha-ready")) + ready := f.postGitea(t, "pull_request", "ready-review", payload(false, strings.Repeat("a", 40))) ready.Body.Close() runs, err = f.st.ListRunsByService(context.Background(), f.serviceID, 10) if err != nil || len(runs) != 1 { @@ -381,7 +381,7 @@ func TestPluginReviewMentionIsAuthorizedAndRepeatable(t *testing.T) { switch { case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/acme/repo/pulls/7": w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"number":7,"html_url":"https://gitea.example/acme/repo/pulls/7","state":"open","head":{"ref":"feature"},"base":{"ref":"main"}}`)) + _, _ = w.Write([]byte(`{"number":7,"html_url":"https://gitea.example/acme/repo/pulls/7","state":"open","head":{"ref":"feature","sha":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},"base":{"ref":"main","sha":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}}`)) default: http.Error(w, "unexpected "+r.Method+" "+r.URL.Path, http.StatusNotFound) } @@ -477,18 +477,42 @@ func TestPluginReviewMentionIsAuthorizedAndRepeatable(t *testing.T) { } func TestPluginGitLabWebhookUsesPerBindingTokenAndDispatches(t *testing.T) { + lookupCalls := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/merge_requests/7") { + lookupCalls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"iid":7,"web_url":"https://gitlab.example/acme/repo/-/merge_requests/7","state":"opened","source_branch":"feature","target_branch":"main","diff_refs":{"head_sha":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","base_sha":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}}`)) + return + } + http.Error(w, "unexpected "+r.Method+" "+r.URL.Path, http.StatusNotFound) + })) + t.Cleanup(upstream.Close) ts, st, cfg := newCipherServer(t, nil, "") ctx := context.Background() if err := st.UpsertProviderConfig(ctx, &domain.ProviderConfig{ - Provider: domain.PluginGitLab, BaseURL: "https://gitlab.example.test", PluginEnabled: true, + Provider: domain.PluginGitLab, BaseURL: upstream.URL, PluginEnabled: true, }); err != nil { t.Fatal(err) } projectID := newProject(t, ts, "gitlab-webhook") now := time.Now().UTC() + cipher, err := auth.NewCipher(cfg.MasterKey) + if err != nil { + t.Fatal(err) + } + accessToken, err := cipher.EncryptString("gitlab-access-token") + if err != nil { + t.Fatal(err) + } + providerConfig, err := st.GetProviderConfig(ctx, domain.PluginGitLab) + if err != nil { + t.Fatal(err) + } installation := &domain.PluginInstallation{ ID: domain.NewID(), ProjectID: projectID, Provider: domain.PluginGitLab, Status: domain.PluginStatusEnabled, ConsentVersion: "v1", ConsentedAt: now, CreatedAt: now, + AccessTokenEnc: accessToken, ConfigRevision: providerConfig.ConfigRevision, } if err := st.CreatePluginInstallation(ctx, installation); err != nil { t.Fatal(err) @@ -516,10 +540,6 @@ func TestPluginGitLabWebhookUsesPerBindingTokenAndDispatches(t *testing.T) { []domain.SCMAction{{AutomationID: automation.ID, ServiceID: svc.ID, EventFamily: "push", Action: "updated"}}, nil, nil); err != nil { t.Fatal(err) } - cipher, err := auth.NewCipher(cfg.MasterKey) - if err != nil { - t.Fatal(err) - } const hookSecret = "gitlab-per-binding-token" secretEnc, err := cipher.EncryptString(hookSecret) if err != nil { @@ -555,6 +575,51 @@ func TestPluginGitLabWebhookUsesPerBindingTokenAndDispatches(t *testing.T) { if err != nil || len(runs) != 1 || runs[0].BaseBranch != "main" { t.Fatalf("GitLab runs=%+v err=%v", runs, err) } + + reviewAutomation := &domain.PluginAutomation{ + ID: domain.NewID(), ServiceID: svc.ID, InstallationID: installation.ID, + Name: "gitlab-review", TriggerKind: "scm", RunKind: domain.RunKindReview, + PromptTemplate: "review {{repository}}", Enabled: true, CreatedAt: now, + } + if err := st.CreatePluginAutomation(ctx, reviewAutomation, &domain.SCMTrigger{AutomationID: reviewAutomation.ID}, + []domain.SCMAction{{AutomationID: reviewAutomation.ID, ServiceID: svc.ID, EventFamily: "pull_request", Action: "synchronized"}}, nil, nil); err != nil { + t.Fatal(err) + } + mrPayload := []byte(`{"object_kind":"merge_request","user":{"id":8,"username":"dev"},"project":{"id":42,"path_with_namespace":"acme/repo","default_branch":"main"},"object_attributes":{"id":70,"iid":7,"action":"update","oldrev":"before-sha","source_branch":"feature","target_branch":"main","url":"https://gitlab.example/acme/repo/-/merge_requests/7"}}`) + mrReq, err := http.NewRequest(http.MethodPost, ts.URL+"/webhooks/gitlab/"+hookID, bytes.NewReader(mrPayload)) + if err != nil { + t.Fatal(err) + } + mrReq.Header.Set("X-Gitlab-Event", "Merge Request Hook") + mrReq.Header.Set("X-Gitlab-Event-UUID", "gitlab-review-delivery") + mrReq.Header.Set("X-Gitlab-Token", hookSecret) + mrResp, err := http.DefaultClient.Do(mrReq) + if err != nil { + t.Fatal(err) + } + if mrResp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(mrResp.Body) + mrResp.Body.Close() + t.Fatalf("GitLab review webhook status=%d body=%s", mrResp.StatusCode, body) + } + mrResp.Body.Close() + runs, err = st.ListRunsByService(ctx, svc.ID, 10) + if err != nil { + t.Fatal(err) + } + var reviewRun *domain.Run + for i := range runs { + if runs[i].Kind == domain.RunKindReview { + reviewRun = &runs[i] + break + } + } + if reviewRun == nil || reviewRun.PRHeadSHA != strings.Repeat("a", 40) || reviewRun.PRBaseSHA != strings.Repeat("b", 40) { + t.Fatalf("GitLab review revision was not enriched before queue: %+v", reviewRun) + } + if lookupCalls != 1 { + t.Fatalf("GitLab PR detail lookups=%d want 1", lookupCalls) + } } func TestGitHubSCMAutomationCreatesRunnableAutomationOriginRun(t *testing.T) { diff --git a/orchestrator/internal/api/review.go b/orchestrator/internal/api/review.go index ecb8b2b0..aae6cfdd 100644 --- a/orchestrator/internal/api/review.go +++ b/orchestrator/internal/api/review.go @@ -66,8 +66,18 @@ func (s *Server) handleRequestReview(w http.ResponseWriter, r *http.Request) { if !s.integrationDispatchAllowed(w, r, svc) { return } + pr, err := s.reviewTargetAtCreation(r.Context(), src, svc) + if err != nil || pr == nil || pr.HeadRef == "" || pr.BaseRef == "" || pr.HeadSHA == "" || pr.BaseSHA == "" { + s.log.Warn("resolve exact review target", "run", src.ID, "err", err) + writeError(w, http.StatusConflict, "review_revision_unavailable", "could not freeze the pull request base/head revisions") + return + } review := newReviewRun(src, svc, principalFrom(r.Context()).userIDPtr()) + review.PRHeadBranch = pr.HeadRef + review.PRBaseBranch = pr.BaseRef + review.PRHeadSHA = pr.HeadSHA + review.PRBaseSHA = pr.BaseSHA review.ModelID = modelID review.ModelName = modelName provenance.Stamp(r.Context(), s.st, review, nil) @@ -80,6 +90,44 @@ func (s *Server) handleRequestReview(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, review) } +// reviewTargetAtCreation reads the pull request before the review Run exists so +// its branch names and exact commit pair become immutable Run input. Plugin +// repositories use their configured base URL; unbound legacy services retain +// the existing credential/factory path. +func (s *Server) reviewTargetAtCreation(ctx context.Context, src *domain.Run, svc *domain.Service) (*provider.PR, error) { + if src.PRNumber <= 0 || svc.RepoKind != domain.RepoKindProvider { + return nil, errors.New("review target is not a provider pull request") + } + repositoryPath := svc.RepoOwnerName + var client provider.Provider + if binding, err := s.st.GetServiceRepositoryBinding(ctx, svc.ID); err == nil { + repositoryPath = binding.RepositoryPath + client, err = s.pluginProviderClient(ctx, binding) + if err != nil { + return nil, err + } + } else if !errors.Is(err, store.ErrNotFound) { + return nil, err + } else { + if s.factory == nil || s.creds == nil { + return nil, errors.New("provider credential resolver is unavailable") + } + tok, err := s.creds.ResolveForService(ctx, svc, src.TriggeredByUserID) + if err != nil { + return nil, err + } + client, err = s.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) + if err != nil { + return nil, err + } + } + owner, repo, ok := provider.SplitRepo(repositoryPath) + if !ok { + return nil, errors.New("provider repository path is invalid") + } + return client.PRByNumber(ctx, owner, repo, src.PRNumber) +} + // newReviewRun builds a queued review run associated with the agent run's PR: it // diffs pr_base_branch...pr_head_branch (the runner reads these from PR_BASE / // PR_HEAD env, injected by the reconciler from these columns), and the reconcile @@ -97,6 +145,9 @@ func newReviewRun(src *domain.Run, svc *domain.Service, triggeredBy *string) *do TriggeredByUserID: triggeredBy, PRHeadBranch: src.GitBranch, PRBaseBranch: svc.DefaultBranch, + PRHeadSHA: src.CommitSHA, + PRURL: src.PRURL, + PRNumber: src.PRNumber, Attempt: 1, CreatedAt: time.Now().UTC(), } diff --git a/orchestrator/internal/api/review_test.go b/orchestrator/internal/api/review_test.go index 711f9a9b..afd961fa 100644 --- a/orchestrator/internal/api/review_test.go +++ b/orchestrator/internal/api/review_test.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -103,7 +104,7 @@ func setupReview(t *testing.T, factory provider.Factory) reviewFixture { agent := &domain.Run{ ID: domain.NewID(), ProjectID: p.ID, ServiceID: svc.ID, Prompt: "add hello", Status: domain.StatusSucceeded, Kind: domain.RunKindAgent, - GitBranch: "jcode/run-abc12345", PRURL: "http://gitea.test/jcloud/seed/pulls/7", PRNumber: 7, + GitBranch: "pr-7-head", PRURL: "http://gitea.test/jcloud/seed/pulls/7", PRNumber: 7, Attempt: 1, CreatedAt: time.Now().UTC(), } if err := st.CreateRun(ctx, agent); err != nil { @@ -127,7 +128,7 @@ func setupReview(t *testing.T, factory provider.Factory) reviewFixture { // TestRequestReviewRBAC: viewer/stranger are forbidden; member/owner/admin/service // may request a review of a succeeded agent run with a PR. func TestRequestReviewRBAC(t *testing.T) { - f := setupReview(t, nil) + f := setupReview(t, &fakePRFactory{prov: provider.NewFakeProvider()}) url := f.ts.URL + "/api/v1/runs/" + f.agentRun.ID + "/review" want := map[string]int{ @@ -146,7 +147,7 @@ func TestRequestReviewRBAC(t *testing.T) { // TestRequestReviewCreatesReviewRun: the created run is kind=review, carries the // PR head/base branches, records the triggering user, and its prompt names the PR. func TestRequestReviewCreatesReviewRun(t *testing.T) { - f := setupReview(t, nil) + f := setupReview(t, &fakePRFactory{prov: provider.NewFakeProvider()}) r := do(t, "POST", f.ts.URL+"/api/v1/runs/"+f.agentRun.ID+"/review", f.tokens["member"], nil) if r.StatusCode != http.StatusCreated { t.Fatalf("request review: status=%d want 201", r.StatusCode) @@ -162,6 +163,12 @@ func TestRequestReviewCreatesReviewRun(t *testing.T) { if run.PRBaseBranch != f.svc.DefaultBranch { t.Errorf("pr_base_branch=%q want %q", run.PRBaseBranch, f.svc.DefaultBranch) } + if run.PRHeadSHA == "" || run.PRBaseSHA == "" { + t.Errorf("review revision pair was not frozen: head=%q base=%q", run.PRHeadSHA, run.PRBaseSHA) + } + if run.PRNumber != f.agentRun.PRNumber || run.PRURL != f.agentRun.PRURL { + t.Errorf("review target=%d %q want %d %q", run.PRNumber, run.PRURL, f.agentRun.PRNumber, f.agentRun.PRURL) + } if run.Status != domain.StatusQueued { t.Errorf("status=%q want queued", run.Status) } @@ -199,7 +206,8 @@ func TestRequestReviewPreconditions(t *testing.T) { // A review run (cannot review a review). rev := &domain.Run{ ID: domain.NewID(), ProjectID: f.projectID, ServiceID: f.svc.ID, Prompt: "x", - Status: domain.StatusSucceeded, Kind: domain.RunKindReview, Attempt: 1, CreatedAt: time.Now().UTC(), + Status: domain.StatusSucceeded, Kind: domain.RunKindReview, Attempt: 1, + PRHeadSHA: strings.Repeat("a", 40), PRBaseSHA: strings.Repeat("b", 40), CreatedAt: time.Now().UTC(), } if err := f.st.CreateRun(ctx, rev); err != nil { t.Fatal(err) @@ -287,7 +295,8 @@ func TestGetPRReviewRunsFilter(t *testing.T) { other := &domain.Run{ ID: domain.NewID(), ProjectID: f.projectID, ServiceID: f.svc.ID, Prompt: "other", Status: domain.StatusSucceeded, Kind: domain.RunKindReview, - PRHeadBranch: "jcode/run-zzz", Attempt: 1, CreatedAt: time.Now().UTC(), + PRHeadBranch: "jcode/run-zzz", PRHeadSHA: strings.Repeat("a", 40), PRBaseSHA: strings.Repeat("b", 40), + Attempt: 1, CreatedAt: time.Now().UTC(), } if err := f.st.CreateRun(ctx, other); err != nil { t.Fatal(err) diff --git a/orchestrator/internal/api/runs.go b/orchestrator/internal/api/runs.go index c2853254..83ddf41b 100644 --- a/orchestrator/internal/api/runs.go +++ b/orchestrator/internal/api/runs.go @@ -211,7 +211,7 @@ func (s *Server) handleListServiceRuns(w http.ResponseWriter, r *http.Request) { if runs == nil { runs = []domain.Run{} } - writeJSON(w, http.StatusOK, map[string]any{"runs": runs}) + writeJSON(w, http.StatusOK, map[string]any{"runs": nonNilRuns(runs)}) } // deref returns the pointed-to string, or "" for a nil pointer. @@ -316,7 +316,13 @@ func nonNilRuns(runs []domain.Run) []domain.Run { if runs == nil { return []domain.Run{} } - return runs + // List views never need embedded profile instructions. Keep them in the + // member-only detail projection and make list payloads uniformly public. + out := make([]domain.Run, len(runs)) + for i := range runs { + out[i] = *projectRunForRole(&runs[i], domain.RoleViewer) + } + return out } func (s *Server) handleGetRun(w http.ResponseWriter, r *http.Request) { @@ -329,9 +335,16 @@ func (s *Server) handleGetRun(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, "internal", "could not get run") return } - if !s.authorizeProject(r.Context(), w, principalFrom(r.Context()), run.ProjectID, domain.RoleViewer) { + prin := principalFrom(r.Context()) + if !s.authorizeProject(r.Context(), w, prin, run.ProjectID, domain.RoleViewer) { + return + } + role, _, err := s.effectiveRole(r.Context(), prin, run.ProjectID) + if err != nil { + writeError(w, http.StatusInternalServerError, "internal", "could not resolve project access") return } + run = projectRunForRole(run, role) usage, err := s.st.GetUsageSummary(r.Context(), domain.UsageSummaryQuery{ SubjectKind: domain.UsageSubjectRun, SubjectID: run.ID, }) @@ -348,6 +361,21 @@ func (s *Server) handleGetRun(w http.ResponseWriter, r *http.Request) { }) } +func projectRunForRole(run *domain.Run, role domain.Role) *domain.Run { + if run == nil || role.AtLeast(domain.RoleMember) || run.ExecutionContract == nil { + return run + } + copyRun := *run + copyContract := *run.ExecutionContract + copyContract.Requirements = append([]string(nil), run.ExecutionContract.Requirements...) + copyContract.Delivery.Outputs = append([]domain.WorkflowOutput(nil), run.ExecutionContract.Delivery.Outputs...) + copyContract.Verification.RequiredRecords = append([]string(nil), run.ExecutionContract.Verification.RequiredRecords...) + copyContract.Profile = run.ExecutionContract.Profile + copyContract.Profile.Instructions = "" + copyRun.ExecutionContract = ©Contract + return ©Run +} + func (s *Server) handleCancelRun(w http.ResponseWriter, r *http.Request) { run, err := s.st.GetRun(r.Context(), r.PathValue("id")) if errors.Is(err, store.ErrNotFound) { @@ -458,6 +486,12 @@ func (s *Server) handleRetryRun(w http.ResponseWriter, r *http.Request) { retry.Kind = orig.Kind retry.PRHeadBranch = orig.PRHeadBranch retry.PRBaseBranch = orig.PRBaseBranch + retry.PRHeadSHA = orig.PRHeadSHA + retry.PRBaseSHA = orig.PRBaseSHA + retry.PRReadyPolicy = orig.PRReadyPolicy + // A retry is another attempt at the same immutable execution contract. A + // genuinely new trigger resolves the current built-in definition instead. + retry.ExecutionContract = orig.ExecutionContract // Session-ness is part of that identity too (D22): retrying a session run // starts a fresh session (same prompt, new ACP session), not a single-shot. retry.Session = orig.Session diff --git a/orchestrator/internal/api/runs_projection_test.go b/orchestrator/internal/api/runs_projection_test.go new file mode 100644 index 00000000..13e8b5d3 --- /dev/null +++ b/orchestrator/internal/api/runs_projection_test.go @@ -0,0 +1,28 @@ +package api + +import ( + "testing" + + "github.com/cnjack/jcloud/internal/domain" +) + +func TestWorkflowContractProfileInstructionsAreMemberOnly(t *testing.T) { + run := &domain.Run{ID: "run", ExecutionContract: &domain.WorkflowContract{ + Profile: domain.AgentProfileSnapshot{ID: domain.BuiltinReviewerProfileID, Instructions: "private bounded instructions"}, + }} + viewer := projectRunForRole(run, domain.RoleViewer) + if viewer.ExecutionContract.Profile.Instructions != "" { + t.Fatal("viewer projection leaked profile instructions") + } + member := projectRunForRole(run, domain.RoleMember) + if member.ExecutionContract.Profile.Instructions != "private bounded instructions" { + t.Fatal("member projection dropped profile instructions") + } + if run.ExecutionContract.Profile.Instructions != "private bounded instructions" { + t.Fatal("projection mutated the stored Run") + } + lists := nonNilRuns([]domain.Run{*run}) + if lists[0].ExecutionContract.Profile.Instructions != "" { + t.Fatal("list projection leaked profile instructions") + } +} diff --git a/orchestrator/internal/api/source.go b/orchestrator/internal/api/source.go index e50a4e1a..726fb6ca 100644 --- a/orchestrator/internal/api/source.go +++ b/orchestrator/internal/api/source.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "sync" "time" @@ -25,6 +26,10 @@ const maxBundleBytes = 16 << 20 // maxReviewBytes caps a review-output upload (defensive; reviews are small). const maxReviewBytes = 1 << 20 +// maxReviewPlanBytes allows a bounded unified diff plus its small JSON +// envelope. The domain parser applies the tighter decoded diff/file/hunk caps. +const maxReviewPlanBytes = domain.MaxReviewDiffBytes + (64 << 10) + // sourceCache serves orchestrator-generated source bundles, generating each // lazily on first request and caching it on disk with a TTL. A per-key mutex // guards generation so concurrent requests for the same run build the bundle @@ -133,19 +138,44 @@ func (s *Server) handleGetSource(w http.ResponseWriter, r *http.Request, runID s return } rawURL := domain.ServiceCloneURL(*svc, s.cfg.GiteaURL) - if binding, bindingErr := s.st.GetServiceRepositoryBinding(r.Context(), svc.ID); bindingErr == nil { - rawURL = binding.CloneURL - } else if !errors.Is(bindingErr, store.ErrNotFound) { - writeError(w, http.StatusInternalServerError, "internal", "could not load repository binding") + resolvedProvider := svc.Provider + var tok credentials.Token + usedFrozenGrant := false + snapshots, snapshotErr := s.st.ListRunPluginSnapshots(r.Context(), runID) + if snapshotErr != nil { + writeError(w, http.StatusInternalServerError, "internal", "could not load Run SCM grant") return } + for i := range snapshots { + snapshot := &snapshots[i] + if !snapshot.HasFrozenRepositoryGrant() { + continue + } + if s.pluginCredentialIssuer == nil { + writeError(w, http.StatusConflict, "scm_grant_unavailable", "the frozen Run SCM grant cannot issue a credential") + return + } + credential, issueErr := s.pluginCredentialIssuer.IssueRunPluginSnapshotCredential(r.Context(), snapshot) + if issueErr != nil { + writeError(w, http.StatusConflict, "scm_grant_unavailable", "the frozen Run SCM grant cannot issue a credential") + return + } + rawURL, resolvedProvider = snapshot.CloneURL, domain.GitProvider(snapshot.Provider) + tok = credentials.Token{Value: credential.AccessToken, Scheme: credential.Scheme, Source: "plugin_snapshot"} + usedFrozenGrant = true + break + } if rawURL == "" { writeError(w, http.StatusInternalServerError, "internal", "could not derive repository URL") return } - // Resolve a credential (integration bot token when the service is bound, else - // user OAuth / gitea PAT). - tok, rerr := s.creds.ResolveForService(r.Context(), svc, run.TriggeredByUserID) + // Legacy non-Plugin Services retain the historical credential resolver. A + // Plugin-bound Run must use the immutable dispatch snapshot above and never + // re-read a current binding or credential after reconnect/rename. + var rerr error + if !usedFrozenGrant { + tok, rerr = s.creds.ResolveForService(r.Context(), svc, run.TriggeredByUserID) + } if rerr != nil && errors.Is(rerr, credentials.ErrIntegrationCredential) { // Fail-visible (F5 review C2): an integration-bound service whose bot token // cannot be used must fail with the REAL reason — never degrade to an @@ -165,10 +195,10 @@ func (s *Server) handleGetSource(w http.ResponseWriter, r *http.Request, runID s // Any OTHER resolution miss (legacy path, no credential at all) is non-fatal: // tok stays the zero value → an anonymous URL (public repos still clone; a // private one fails visibly at git clone time). - authed := tok.AuthedURL(rawURL, svc.Provider) + authed := tok.AuthedURL(rawURL, resolvedProvider) data, err := s.srcCache.Get(runID, func(dst string) error { - if run.Kind == domain.RunKindReview && svc.Provider == domain.ProviderGitHub && + if run.Kind == domain.RunKindReview && resolvedProvider == domain.ProviderGitHub && run.PRNumber > 0 && run.PRHeadBranch != "" { return s.git.CreatePullRequestSourceBundle(r.Context(), authed, dst, run.PRNumber, run.PRHeadBranch) } @@ -241,6 +271,74 @@ func (s *Server) handleIngestBundle(w http.ResponseWriter, r *http.Request, runI const structuredReviewMediaType = "application/vnd.jcode.review+json" +// handleIngestReviewPlan accepts the Runner's exact revision facts and raw +// unified diff before the Agent turn. The control plane, rather than the Runner, +// parses changed-line anchors and computes the canonical plan hash. +func (s *Server) handleIngestReviewPlan(w http.ResponseWriter, r *http.Request, runID string) { + data, err := io.ReadAll(io.LimitReader(r.Body, int64(maxReviewPlanBytes)+1)) + if err != nil { + writeError(w, http.StatusBadRequest, "bad_request", "could not read review plan") + return + } + if len(data) > maxReviewPlanBytes { + writeError(w, http.StatusRequestEntityTooLarge, "review_input_too_large", "review plan exceeds the size limit") + return + } + var input domain.ReviewPlanInput + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&input); err != nil || decoder.Decode(&struct{}{}) != io.EOF { + writeError(w, http.StatusBadRequest, "invalid_review_plan", "review plan must be exactly one valid JSON object") + return + } + run, err := s.st.GetRun(r.Context(), runID) + if errors.Is(err, store.ErrNotFound) { + writeError(w, http.StatusNotFound, "not_found", "run not found") + return + } + if err != nil { + writeError(w, http.StatusInternalServerError, "internal", "could not load run") + return + } + if run.Kind != domain.RunKindReview || run.PRBaseSHA == "" || run.PRHeadSHA == "" { + writeError(w, http.StatusConflict, "review_revision_unavailable", "review run has no frozen base/head revision pair") + return + } + if input.BaseSHA != run.PRBaseSHA || input.HeadSHA != run.PRHeadSHA { + writeError(w, http.StatusConflict, "review_revision_mismatch", "review plan revisions do not match the frozen Run revisions") + return + } + input.CreatedAt = time.Now().UTC() + plan, err := domain.BuildReviewPlan(input) + if err != nil { + if strings.Contains(err.Error(), "review_input_too_large") { + writeError(w, http.StatusRequestEntityTooLarge, "review_input_too_large", err.Error()) + } else { + writeError(w, http.StatusBadRequest, "invalid_review_plan", err.Error()) + } + return + } + committed, created, err := s.st.SetReviewPlan(r.Context(), runID, *plan) + if errors.Is(err, store.ErrConflict) { + writeError(w, http.StatusConflict, "review_plan_conflict", "a different review plan is already stored for this Run") + return + } + if errors.Is(err, store.ErrInvalidTransition) { + writeError(w, http.StatusConflict, "review_plan_closed", "the Run is not accepting a review plan") + return + } + if err != nil { + s.log.Error("ingest review plan", "run", runID, "err", err) + writeError(w, http.StatusInternalServerError, "internal", "could not store review plan") + return + } + status := http.StatusOK + if created { + status = http.StatusCreated + } + writeJSON(w, status, committed.ReviewPlan) +} + // handleIngestReview accepts validated structured review output from new // runners while retaining text/plain for rolling-upgrade compatibility. func (s *Server) handleIngestReview(w http.ResponseWriter, r *http.Request, runID string) { @@ -258,6 +356,19 @@ func (s *Server) handleIngestReview(w http.ResponseWriter, r *http.Request, runI return } mediaType, _, _ := mime.ParseMediaType(r.Header.Get("Content-Type")) + run, getErr := s.st.GetRun(r.Context(), runID) + if errors.Is(getErr, store.ErrNotFound) { + writeError(w, http.StatusNotFound, "not_found", "run not found") + return + } + if getErr != nil { + writeError(w, http.StatusInternalServerError, "internal", "could not load run") + return + } + if run.ExecutionContract != nil && run.Kind == domain.RunKindReview && mediaType != structuredReviewMediaType { + writeError(w, http.StatusConflict, "structured_review_required", "contract review runs accept only structured review output") + return + } if mediaType == structuredReviewMediaType { var result domain.ReviewResult decoder := json.NewDecoder(bytes.NewReader(data)) @@ -270,10 +381,23 @@ func (s *Server) handleIngestReview(w http.ResponseWriter, r *http.Request, runI writeError(w, http.StatusBadRequest, "invalid_review_result", "review output must contain exactly one JSON object") return } - if err := result.Validate(); err != nil { - writeError(w, http.StatusBadRequest, "invalid_review_result", err.Error()) + if run.ExecutionContract != nil && run.Kind == domain.RunKindReview && run.ReviewPlan == nil { + writeError(w, http.StatusConflict, "review_plan_required", "deterministic review output requires an accepted review plan") return } + if run.ReviewPlan != nil { + if err := result.ValidateAgainst(run.ReviewPlan); err != nil { + writeError(w, http.StatusBadRequest, "invalid_review_result", err.Error()) + return + } + } else { + // Rolling-upgrade compatibility for historical Runs which predate the + // deterministic Plan. New contract Runs are rejected above when absent. + if err := result.Validate(); err != nil { + writeError(w, http.StatusBadRequest, "invalid_review_result", err.Error()) + return + } + } if _, err := s.st.SetReviewResult(r.Context(), runID, result); err != nil { s.writeReviewStoreError(w, runID, err) return diff --git a/orchestrator/internal/api/source_review_test.go b/orchestrator/internal/api/source_review_test.go index eff4b09f..b084b4b1 100644 --- a/orchestrator/internal/api/source_review_test.go +++ b/orchestrator/internal/api/source_review_test.go @@ -3,10 +3,12 @@ package api import ( "bytes" "context" + "encoding/json" "io" "log/slog" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -34,11 +36,26 @@ func TestIngestStructuredReviewValidatesBeforePersisting(t *testing.T) { run := &domain.Run{ ID: domain.NewID(), ProjectID: project.ID, ServiceID: service.ID, Prompt: "review", Status: domain.StatusRunning, Kind: domain.RunKindReview, CreatedAt: time.Now(), + PRBaseSHA: strings.Repeat("a", 40), PRHeadSHA: strings.Repeat("b", 40), } if err := st.CreateRun(ctx, run); err != nil { t.Fatal(err) } server := New(st, &config.Config{}, slog.New(slog.NewTextHandler(io.Discard, nil)), sse.NewHub(), nil) + withoutPlanReq := httptest.NewRequest(http.MethodPost, "/internal/v1/runs/"+run.ID+"/review", bytes.NewBufferString(`{"summary":"No findings.","findings":[]}`)) + withoutPlanReq.Header.Set("Content-Type", structuredReviewMediaType) + withoutPlanResp := httptest.NewRecorder() + server.handleIngestReview(withoutPlanResp, withoutPlanReq, run.ID) + if withoutPlanResp.Code != http.StatusConflict || !strings.Contains(withoutPlanResp.Body.String(), "review_plan_required") { + t.Fatalf("review without plan status=%d body=%s", withoutPlanResp.Code, withoutPlanResp.Body.String()) + } + planBody := `{"base_sha":"` + run.PRBaseSHA + `","head_sha":"` + run.PRHeadSHA + `","merge_base_sha":"` + strings.Repeat("c", 40) + `","diff":"diff --git a/ledger.py b/ledger.py\n--- a/ledger.py\n+++ b/ledger.py\n@@ -8 +8 @@\n-old\n+new\n"}` + planReq := httptest.NewRequest(http.MethodPost, "/internal/v1/runs/"+run.ID+"/review-plan", bytes.NewBufferString(planBody)) + planResp := httptest.NewRecorder() + server.handleIngestReviewPlan(planResp, planReq, run.ID) + if planResp.Code != http.StatusCreated { + t.Fatalf("review plan status=%d body=%s", planResp.Code, planResp.Body.String()) + } post := func(body string) *httptest.ResponseRecorder { request := httptest.NewRequest(http.MethodPost, "/internal/v1/runs/"+run.ID+"/review", bytes.NewBufferString(body)) @@ -69,3 +86,80 @@ func TestIngestStructuredReviewValidatesBeforePersisting(t *testing.T) { t.Fatal("invalid structured upload replaced the validated result") } } + +func TestIngestReviewPlanIsFirstWriteAndRejectsRevisionMismatch(t *testing.T) { + ctx := context.Background() + st := store.NewMemStore() + base, head := strings.Repeat("a", 40), strings.Repeat("b", 40) + run := &domain.Run{ID: domain.NewID(), ProjectID: "p", ServiceID: "s", Prompt: "review", Status: domain.StatusRunning, Kind: domain.RunKindReview, PRBaseSHA: base, PRHeadSHA: head, CreatedAt: time.Now()} + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + server := New(st, &config.Config{}, slog.New(slog.NewTextHandler(io.Discard, nil)), sse.NewHub(), nil) + post := func(baseSHA, diff string) *httptest.ResponseRecorder { + body := `{"base_sha":"` + baseSHA + `","head_sha":"` + head + `","merge_base_sha":"` + strings.Repeat("c", 40) + `","diff":` + string(mustJSON(t, diff)) + `}` + req := httptest.NewRequest(http.MethodPost, "/internal/v1/runs/"+run.ID+"/review-plan", bytes.NewBufferString(body)) + response := httptest.NewRecorder() + server.handleIngestReviewPlan(response, req, run.ID) + return response + } + diff := "diff --git a/a.go b/a.go\n--- a/a.go\n+++ b/a.go\n@@ -1 +1 @@\n-old\n+new\n" + if response := post(base, diff); response.Code != http.StatusCreated { + t.Fatalf("first status=%d body=%s", response.Code, response.Body.String()) + } + if response := post(base, diff); response.Code != http.StatusOK { + t.Fatalf("retry status=%d body=%s", response.Code, response.Body.String()) + } + if response := post(strings.Repeat("d", 40), diff); response.Code != http.StatusConflict { + t.Fatalf("mismatch status=%d body=%s", response.Code, response.Body.String()) + } +} + +func TestContractReviewRejectsLegacyMarkdownOutput(t *testing.T) { + ctx := context.Background() + st := store.NewMemStore() + project := &domain.Project{ID: domain.NewID(), Name: "review", CreatedAt: time.Now()} + if err := st.CreateProject(ctx, project); err != nil { + t.Fatal(err) + } + service := &domain.Service{ + ID: domain.NewID(), ProjectID: project.ID, Name: "repo", RepoKind: domain.RepoKindProvider, + Provider: domain.ProviderGitHub, RepoOwnerName: "acme/repo", DefaultBranch: "main", + GitMode: domain.GitModeReadonly, CreatedAt: time.Now(), + } + if err := st.CreateService(ctx, service); err != nil { + t.Fatal(err) + } + run := &domain.Run{ + ID: domain.NewID(), ProjectID: project.ID, ServiceID: service.ID, Prompt: "review", + Status: domain.StatusRunning, Kind: domain.RunKindReview, CreatedAt: time.Now(), + PRBaseSHA: strings.Repeat("a", 40), PRHeadSHA: strings.Repeat("b", 40), + } + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + server := New(st, &config.Config{}, slog.New(slog.NewTextHandler(io.Discard, nil)), sse.NewHub(), nil) + req := httptest.NewRequest(http.MethodPost, "/internal/v1/runs/"+run.ID+"/review", strings.NewReader("legacy markdown")) + req.Header.Set("Content-Type", "text/plain") + response := httptest.NewRecorder() + server.handleIngestReview(response, req, run.ID) + if response.Code != http.StatusConflict || !strings.Contains(response.Body.String(), "structured_review_required") { + t.Fatalf("status=%d body=%s", response.Code, response.Body.String()) + } + stored, err := st.GetRun(ctx, run.ID) + if err != nil { + t.Fatal(err) + } + if stored.ReviewOutput != "" { + t.Fatal("legacy markdown was persisted for a contract review") + } +} + +func mustJSON(t *testing.T, value string) []byte { + t.Helper() + data, err := json.Marshal(value) + if err != nil { + t.Fatal(err) + } + return data +} diff --git a/orchestrator/internal/api/webhook.go b/orchestrator/internal/api/webhook.go index 513341fc..522a8f9d 100644 --- a/orchestrator/internal/api/webhook.go +++ b/orchestrator/internal/api/webhook.go @@ -100,6 +100,7 @@ type giteaPullRequestPayload struct { } `json:"head"` Base struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"base"` } `json:"pull_request"` Repository struct { @@ -116,6 +117,7 @@ type webhookReviewEvent struct { headRef string headSHA string baseRef string + baseSHA string draft bool } @@ -325,13 +327,13 @@ func parseGiteaReviewEvent(header string, body []byte) (*webhookReviewEvent, str return nil, "ignored: PR action is not an Automation event" } if p.Repository.FullName == "" || p.Number == 0 || p.PullRequest.Head.Ref == "" || - p.PullRequest.Head.SHA == "" || p.PullRequest.Base.Ref == "" { + p.PullRequest.Head.SHA == "" || p.PullRequest.Base.Ref == "" || p.PullRequest.Base.SHA == "" { return nil, "ignored: incomplete pull-request payload" } return &webhookReviewEvent{ provider: domain.ProviderGitea, repoFullName: p.Repository.FullName, event: event, prNumber: p.Number, prURL: p.PullRequest.HTMLURL, headRef: p.PullRequest.Head.Ref, - headSHA: p.PullRequest.Head.SHA, baseRef: p.PullRequest.Base.Ref, draft: p.PullRequest.Draft, + headSHA: p.PullRequest.Head.SHA, baseRef: p.PullRequest.Base.Ref, baseSHA: p.PullRequest.Base.SHA, draft: p.PullRequest.Draft, }, "" } @@ -403,7 +405,7 @@ func (s *Server) processReviewEvent(ctx context.Context, event *webhookReviewEve Prompt: a.Instructions, Status: domain.StatusQueued, Kind: domain.RunKindReview, Phase: "Queued", TriggeredByUserID: a.CreatedBy, Attempt: 1, CreatedAt: now, PRURL: event.prURL, PRNumber: event.prNumber, PRHeadBranch: event.headRef, - PRBaseBranch: event.baseRef, Origin: domain.RunOriginAutomation, + PRBaseBranch: event.baseRef, PRHeadSHA: event.headSHA, PRBaseSHA: event.baseSHA, Origin: domain.RunOriginAutomation, OriginAutomationID: a.ID, OriginEventKey: key, ModelName: sel.ModelName, } if sel.ModelID != "" { @@ -647,7 +649,8 @@ func (s *Server) processMention(ctx context.Context, m *webhookMention, cmd ment // PR detail (head/base branch + html_url) — the payload's issue omits them. pr, err := client.PRByNumber(ctx, owner, repo, m.prNumber) - if err != nil || pr == nil || pr.HeadRef == "" { + if err != nil || pr == nil || pr.HeadRef == "" || + (cmd.kind == cmdReview && (pr.BaseRef == "" || pr.HeadSHA == "" || pr.BaseSHA == "")) { s.log.Warn("webhook: PR detail lookup failed", "provider", m.provider, "repo", m.repoFullName, "pr", m.prNumber, "err", err) reply("jcode couldn't read this pull request from " + providerDisplayName(m.provider) + ".") return @@ -764,6 +767,8 @@ func newWebhookRun(svc *domain.Service, userID string, cmd mentionCommand, pr *p PRNumber: m.prNumber, PRHeadBranch: pr.HeadRef, PRBaseBranch: pr.BaseRef, + PRHeadSHA: pr.HeadSHA, + PRBaseSHA: pr.BaseSHA, } if cmd.kind == cmdReview { run.Kind = domain.RunKindReview diff --git a/orchestrator/internal/domain/domain.go b/orchestrator/internal/domain/domain.go index f810243f..02c45217 100644 --- a/orchestrator/internal/domain/domain.go +++ b/orchestrator/internal/domain/domain.go @@ -438,6 +438,10 @@ type Run struct { // ReviewResult is validated provider-neutral output. ReviewOutput remains // readable for legacy runners and providers during the transition. ReviewResult *ReviewResult `json:"review_result,omitempty"` + // ReviewPlan is the immutable server-canonicalized input coverage ledger for + // a deterministic review. Its private changed-line anchors are persisted but + // excluded from the public Run JSON projection. + ReviewPlan *ReviewPlan `json:"review_plan,omitempty"` // ReviewPostedAt is stamped once the orchestrator has posted a review run's // output as a PR review comment on the provider (idempotency marker; M3 // reconcile review pass). Nil until posted / for agent runs. @@ -456,6 +460,13 @@ type Run struct { // update-push mode instead of opening a new draft PR (blueprint §8). PRHeadBranch string `json:"pr_head_branch,omitempty"` PRBaseBranch string `json:"pr_base_branch,omitempty"` + PRHeadSHA string `json:"pr_head_sha,omitempty"` + PRBaseSHA string `json:"pr_base_sha,omitempty"` + + // ExecutionContract is resolved once in the same transaction that creates + // the Run. Retry/resume paths copy it explicitly; mutable Project, Service, + // Automation, and profile settings are never consulted to reinterpret it. + ExecutionContract *WorkflowContract `json:"execution_contract,omitempty"` // Origin records how the run was triggered (api|webhook; M7 / blueprint §8). // Defaults to api. OriginCommentID/URL are set only for webhook runs — the diff --git a/orchestrator/internal/domain/plugins.go b/orchestrator/internal/domain/plugins.go index 7b62107f..72d67bd8 100644 --- a/orchestrator/internal/domain/plugins.go +++ b/orchestrator/internal/domain/plugins.go @@ -323,6 +323,17 @@ type RunPluginSnapshot struct { ProviderConfigRevision int64 `json:"-"` CredentialVersionID string `json:"-"` + // Repository grant facts are copied from the service binding in the same + // dispatch transaction as the credential/config revisions. Only the snapshot + // matching this Run's repository carries them; unrelated project Plugin + // snapshots keep these fields empty. + RepositoryID string `json:"repository_id,omitempty"` + RepositoryPath string `json:"repository_path,omitempty"` + CloneURL string `json:"-"` + DefaultBranch string `json:"default_branch,omitempty"` + ActingPrincipalKind string `json:"acting_principal_kind,omitempty"` + ActingPrincipalID string `json:"acting_principal_id,omitempty"` + // The following fields are hydrated from the referenced immutable version // rows by Store.ListRunPluginSnapshots. They are never persisted in // run_plugin_snapshots itself and never serialized to callers. @@ -381,6 +392,11 @@ func (s RunPluginSnapshot) HasFrozenRuntimeMaterial() bool { return len(s.AccessTokenEnc) > 0 } +func (s RunPluginSnapshot) HasFrozenRepositoryGrant() bool { + return s.RepositoryID != "" && s.RepositoryPath != "" && s.CloneURL != "" && + s.DefaultBranch != "" && s.ActingPrincipalKind != "" && s.ActingPrincipalID != "" +} + func clonePluginSnapshotTime(v *time.Time) *time.Time { if v == nil { return nil diff --git a/orchestrator/internal/domain/review_plan.go b/orchestrator/internal/domain/review_plan.go new file mode 100644 index 00000000..9aed6d8a --- /dev/null +++ b/orchestrator/internal/domain/review_plan.go @@ -0,0 +1,321 @@ +package domain + +import ( + "bufio" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "regexp" + "sort" + "strconv" + "strings" + "time" +) + +const ( + ReviewPlanSchemaVersion = 1 + MaxReviewDiffBytes = 2 << 20 + MaxReviewFiles = 400 + MaxReviewHunks = 2_000 + ReviewCoverageComplete = "complete" + ReviewCoveragePartial = "partial" +) + +type ReviewPlanInput struct { + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + MergeBaseSHA string `json:"merge_base_sha"` + Diff string `json:"diff"` + CreatedAt time.Time `json:"-"` +} + +type ReviewPlan struct { + SchemaVersion int `json:"schema_version"` + PlanHash string `json:"plan_hash"` + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + MergeBaseSHA string `json:"merge_base_sha"` + RulesRevision string `json:"rules_revision"` + Coverage string `json:"coverage"` + ChangedFiles int `json:"changed_files"` + EligibleFiles int `json:"eligible_files"` + IndexedFiles int `json:"indexed_files"` + ChangedHunks int `json:"changed_hunks"` + IndexedHunks int `json:"indexed_hunks"` + ChangedLines int `json:"changed_lines"` + Files []ReviewPlanFile `json:"files"` + Anchors []ReviewAnchor `json:"-"` + CreatedAt time.Time `json:"created_at"` +} + +type ReviewPlanFile struct { + Path string `json:"path"` + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + Hunks int `json:"hunks"` + ChangedLines int `json:"changed_lines"` +} + +type ReviewAnchor struct { + Path string `json:"path"` + StartLine int `json:"start_line"` + EndLine int `json:"end_line"` +} + +var hunkHeader = regexp.MustCompile(`^@@ -[0-9]+(?:,[0-9]+)? \+([0-9]+)(?:,([0-9]+))? @@`) + +func BuildReviewPlan(input ReviewPlanInput) (*ReviewPlan, error) { + input.BaseSHA = strings.TrimSpace(input.BaseSHA) + input.HeadSHA = strings.TrimSpace(input.HeadSHA) + input.MergeBaseSHA = strings.TrimSpace(input.MergeBaseSHA) + if !ValidCommitSHA(input.BaseSHA) || !ValidCommitSHA(input.HeadSHA) || !ValidCommitSHA(input.MergeBaseSHA) { + return nil, errors.New("review_revision_invalid: base, head, and merge-base must be commit SHAs") + } + if len(input.Diff) > MaxReviewDiffBytes { + return nil, fmt.Errorf("review_input_too_large: diff exceeds %d bytes", MaxReviewDiffBytes) + } + plan := &ReviewPlan{SchemaVersion: ReviewPlanSchemaVersion, BaseSHA: input.BaseSHA, HeadSHA: input.HeadSHA, MergeBaseSHA: input.MergeBaseSHA, RulesRevision: "review-v2", Coverage: ReviewCoverageComplete, CreatedAt: input.CreatedAt.UTC()} + if plan.CreatedAt.IsZero() { + plan.CreatedAt = time.Now().UTC() + } + type fileState struct { + summary ReviewPlanFile + lines []int + newLine int + inHunk bool + } + var current *fileState + flush := func() error { + if current == nil { + return nil + } + if current.summary.Path == "" || !safeReviewPath(current.summary.Path) { + current.summary.Status, current.summary.Reason = "skipped", "unsupported" + } + if current.summary.Status == "" { + current.summary.Status = "indexed" + } + plan.ChangedFiles++ + if plan.ChangedFiles > MaxReviewFiles { + return fmt.Errorf("review_input_too_large: more than %d changed files", MaxReviewFiles) + } + if current.summary.Status == "indexed" { + plan.EligibleFiles++ + plan.IndexedFiles++ + plan.IndexedHunks += current.summary.Hunks + plan.ChangedLines += len(current.lines) + for _, a := range compressReviewLines(current.summary.Path, current.lines) { + plan.Anchors = append(plan.Anchors, a) + } + } else { + plan.Coverage = ReviewCoveragePartial + } + plan.ChangedHunks += current.summary.Hunks + current.summary.ChangedLines = len(current.lines) + plan.Files = append(plan.Files, current.summary) + current = nil + return nil + } + scanner := bufio.NewScanner(strings.NewReader(input.Diff)) + buffer := make([]byte, 64*1024) + scanner.Buffer(buffer, MaxReviewDiffBytes+1024) + for scanner.Scan() { + line := scanner.Text() + if strings.HasPrefix(line, "diff --git ") { + if err := flush(); err != nil { + return nil, err + } + parts := strings.SplitN(line, " b/", 2) + path := "" + if len(parts) == 2 { + path = strings.Trim(parts[1], `"`) + } + current = &fileState{summary: ReviewPlanFile{Path: path}} + continue + } + if current == nil { + continue + } + if strings.HasPrefix(line, "+++ ") { + value := strings.TrimPrefix(line, "+++ ") + if value != "/dev/null" { + current.summary.Path = strings.TrimPrefix(strings.Trim(value, `"`), "b/") + } + continue + } + if strings.HasPrefix(line, "Binary files ") || strings.HasPrefix(line, "GIT binary patch") { + current.summary.Status, current.summary.Reason = "skipped", "binary" + current.inHunk = false + continue + } + if strings.HasPrefix(line, "Submodule ") { + current.summary.Status, current.summary.Reason = "skipped", "unsupported" + continue + } + if match := hunkHeader.FindStringSubmatch(line); match != nil { + start, _ := strconv.Atoi(match[1]) + current.newLine, current.inHunk = start, true + current.summary.Hunks++ + if plan.ChangedHunks+current.summary.Hunks > MaxReviewHunks { + return nil, fmt.Errorf("review_input_too_large: more than %d hunks", MaxReviewHunks) + } + continue + } + if !current.inHunk || line == "\\ No newline at end of file" { + continue + } + switch { + case strings.HasPrefix(line, "+") && !strings.HasPrefix(line, "+++"): + current.lines = append(current.lines, current.newLine) + current.newLine++ + case strings.HasPrefix(line, "-") && !strings.HasPrefix(line, "---"): + // A deletion has no right-side anchor and does not advance newLine. + default: + current.newLine++ + } + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("parse review diff: %w", err) + } + if err := flush(); err != nil { + return nil, err + } + sort.Slice(plan.Files, func(i, j int) bool { return plan.Files[i].Path < plan.Files[j].Path }) + sort.Slice(plan.Anchors, func(i, j int) bool { + if plan.Anchors[i].Path == plan.Anchors[j].Path { + return plan.Anchors[i].StartLine < plan.Anchors[j].StartLine + } + return plan.Anchors[i].Path < plan.Anchors[j].Path + }) + hash, err := plan.CanonicalHash() + if err != nil { + return nil, err + } + plan.PlanHash = hash + return plan, nil +} + +// ValidCommitSHA accepts full provider commit IDs and abbreviated hexadecimal +// object names. Creation paths use it to reject malformed revision pairs before +// queueing; the Runner still verifies that the objects exist and match exactly. +func ValidCommitSHA(value string) bool { + if len(value) < 7 || len(value) > 64 { + return false + } + for _, r := range value { + if !strings.ContainsRune("0123456789abcdefABCDEF", r) { + return false + } + } + return true +} + +func compressReviewLines(path string, lines []int) []ReviewAnchor { + if len(lines) == 0 { + return nil + } + sort.Ints(lines) + start, end := lines[0], lines[0] + var out []ReviewAnchor + for _, line := range lines[1:] { + if line <= end+1 { + if line > end { + end = line + } + continue + } + out = append(out, ReviewAnchor{Path: path, StartLine: start, EndLine: end}) + start, end = line, line + } + return append(out, ReviewAnchor{Path: path, StartLine: start, EndLine: end}) +} + +func (p ReviewPlan) AllowsAnchor(path string, line, endLine int) bool { + if endLine == 0 { + endLine = line + } + for _, anchor := range p.Anchors { + if anchor.Path == path && line >= anchor.StartLine && endLine <= anchor.EndLine { + return true + } + } + return false +} + +func (r ReviewResult) ValidateAgainst(plan *ReviewPlan) error { + if err := r.Validate(); err != nil { + return err + } + if plan == nil { + return errors.New("review plan is required") + } + for i, finding := range r.Findings { + if !plan.AllowsAnchor(finding.Path, finding.Line, finding.EndLine) { + return fmt.Errorf("finding %d is not anchored to a changed right-side line", i+1) + } + } + return nil +} + +type storedReviewPlan struct { + SchemaVersion int `json:"schema_version"` + PlanHash string `json:"plan_hash"` + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + MergeBaseSHA string `json:"merge_base_sha"` + RulesRevision string `json:"rules_revision"` + Coverage string `json:"coverage"` + ChangedFiles int `json:"changed_files"` + EligibleFiles int `json:"eligible_files"` + IndexedFiles int `json:"indexed_files"` + ChangedHunks int `json:"changed_hunks"` + IndexedHunks int `json:"indexed_hunks"` + ChangedLines int `json:"changed_lines"` + Files []ReviewPlanFile `json:"files"` + Anchors []ReviewAnchor `json:"anchors"` + CreatedAt time.Time `json:"created_at"` +} + +func (p ReviewPlan) stored() storedReviewPlan { + return storedReviewPlan{p.SchemaVersion, p.PlanHash, p.BaseSHA, p.HeadSHA, p.MergeBaseSHA, p.RulesRevision, p.Coverage, p.ChangedFiles, p.EligibleFiles, p.IndexedFiles, p.ChangedHunks, p.IndexedHunks, p.ChangedLines, p.Files, p.Anchors, p.CreatedAt} +} + +func (p ReviewPlan) CanonicalHash() (string, error) { + stored := p.stored() + stored.PlanHash = "" + stored.CreatedAt = time.Time{} + data, err := json.Marshal(stored) + if err != nil { + return "", err + } + sum := sha256.Sum256(data) + return "sha256:" + hex.EncodeToString(sum[:]), nil +} + +func MarshalStoredReviewPlan(p *ReviewPlan) ([]byte, error) { + if p == nil { + return nil, nil + } + return json.Marshal(p.stored()) +} + +func UnmarshalStoredReviewPlan(data []byte) (*ReviewPlan, error) { + if len(data) == 0 || string(data) == "null" { + return nil, nil + } + var value storedReviewPlan + if err := json.Unmarshal(data, &value); err != nil { + return nil, err + } + p := &ReviewPlan{value.SchemaVersion, value.PlanHash, value.BaseSHA, value.HeadSHA, value.MergeBaseSHA, value.RulesRevision, value.Coverage, value.ChangedFiles, value.EligibleFiles, value.IndexedFiles, value.ChangedHunks, value.IndexedHunks, value.ChangedLines, value.Files, value.Anchors, value.CreatedAt} + want, err := p.CanonicalHash() + if err != nil { + return nil, err + } + if p.PlanHash != want { + return nil, errors.New("review plan hash mismatch") + } + return p, nil +} diff --git a/orchestrator/internal/domain/review_plan_test.go b/orchestrator/internal/domain/review_plan_test.go new file mode 100644 index 00000000..090decf9 --- /dev/null +++ b/orchestrator/internal/domain/review_plan_test.go @@ -0,0 +1,91 @@ +package domain + +import ( + "strings" + "testing" + "time" +) + +const reviewPlanFixture = `diff --git a/a.go b/a.go +index 1111111..2222222 100644 +--- a/a.go ++++ b/a.go +@@ -8,3 +8,4 @@ func a() { + context +-old ++new ++another + context +diff --git a/logo.png b/logo.png +new file mode 100644 +index 0000000..3333333 +Binary files /dev/null and b/logo.png differ +diff --git a/renamed.txt b/moved.txt +similarity index 90% +rename from renamed.txt +rename to moved.txt +--- a/renamed.txt ++++ b/moved.txt +@@ -1 +1 @@ +-before ++after +` + +func TestBuildReviewPlanIndexesOnlyRightSideChangedLines(t *testing.T) { + plan, err := BuildReviewPlan(ReviewPlanInput{ + BaseSHA: strings.Repeat("a", 40), HeadSHA: strings.Repeat("b", 40), MergeBaseSHA: strings.Repeat("c", 40), + Diff: reviewPlanFixture, CreatedAt: time.Unix(1, 0).UTC(), + }) + if err != nil { + t.Fatal(err) + } + if plan.Coverage != ReviewCoveragePartial || plan.ChangedFiles != 3 || plan.EligibleFiles != 2 || plan.IndexedFiles != 2 { + t.Fatalf("coverage summary = %#v", plan) + } + if plan.ChangedHunks != 2 || plan.IndexedHunks != 2 || plan.ChangedLines != 3 { + t.Fatalf("hunk summary = %#v", plan) + } + if !plan.AllowsAnchor("a.go", 9, 10) { + t.Fatal("changed right-side range rejected") + } + if plan.AllowsAnchor("a.go", 8, 0) { + t.Fatal("context line accepted") + } + if plan.AllowsAnchor("logo.png", 1, 0) { + t.Fatal("binary line accepted") + } + if !plan.AllowsAnchor("moved.txt", 1, 0) { + t.Fatal("renamed changed line rejected") + } + if plan.PlanHash == "" { + t.Fatal("plan hash missing") + } + if len(plan.Anchors) == 0 { + t.Fatal("private anchors missing") + } +} + +func TestBuildReviewPlanRejectsUnboundedInput(t *testing.T) { + _, err := BuildReviewPlan(ReviewPlanInput{BaseSHA: strings.Repeat("a", 40), HeadSHA: strings.Repeat("b", 40), MergeBaseSHA: strings.Repeat("c", 40), Diff: strings.Repeat("x", MaxReviewDiffBytes+1)}) + if err == nil || !strings.Contains(err.Error(), "review_input_too_large") { + t.Fatalf("err = %v", err) + } +} + +func TestReviewResultValidateAgainstPlan(t *testing.T) { + plan, err := BuildReviewPlan(ReviewPlanInput{BaseSHA: strings.Repeat("a", 40), HeadSHA: strings.Repeat("b", 40), MergeBaseSHA: strings.Repeat("c", 40), Diff: reviewPlanFixture}) + if err != nil { + t.Fatal(err) + } + valid := ReviewResult{Summary: "one issue", Findings: []ReviewFinding{{Path: "a.go", Line: 9, EndLine: 10, Severity: "P1", Confidence: 95, Title: "bug", Body: "breaks behavior"}}} + if err := valid.ValidateAgainst(plan); err != nil { + t.Fatalf("valid result: %v", err) + } + invalid := valid + invalid.Findings = append([]ReviewFinding(nil), valid.Findings...) + invalid.Findings[0].Line = 8 + invalid.Findings[0].EndLine = 0 + if err := invalid.ValidateAgainst(plan); err == nil || !strings.Contains(err.Error(), "changed right-side line") { + t.Fatalf("err = %v", err) + } +} diff --git a/orchestrator/internal/domain/workflow_contract.go b/orchestrator/internal/domain/workflow_contract.go new file mode 100644 index 00000000..0040475e --- /dev/null +++ b/orchestrator/internal/domain/workflow_contract.go @@ -0,0 +1,430 @@ +package domain + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "slices" + "strings" + "time" +) + +const ( + WorkflowContractSchemaVersion = 1 + + BuiltinImplementationTaskID = "builtin:implementation-task" + BuiltinPullRequestReviewID = "builtin:pull-request-review" + BuiltinDeveloperProfileID = "builtin:developer" + BuiltinReviewerProfileID = "builtin:reviewer" + + OutputDiffOnly = "diff_only" + OutputCreatePullRequest = "create_pull_request" + OutputProviderReview = "provider_review" + + WorkspaceReadOnly = "read_only" + WorkspaceReadWrite = "read_write" + TimeoutSourceProject = "project_override" + TimeoutSourceCluster = "cluster_default" + + RequirementSourceRead = "source.read" + RequirementSourceWrite = "source.write" + RequirementGit = "git" + RequirementShell = "shell" + RequirementRipgrep = "ripgrep" + RequirementSCMPullRequestWrite = "scm.pull_request.write" + RequirementSCMReviewWrite = "scm.review.write" +) + +var validWorkflowRequirements = []string{ + RequirementGit, RequirementRipgrep, RequirementSCMPullRequestWrite, + RequirementSCMReviewWrite, RequirementShell, RequirementSourceRead, + RequirementSourceWrite, +} + +type WorkflowContract struct { + SchemaVersion int `json:"schema_version"` + Hash string `json:"hash"` + Workflow WorkflowIdentity `json:"workflow"` + Profile AgentProfileSnapshot `json:"profile"` + Trigger WorkflowTrigger `json:"trigger"` + Execution WorkflowExecution `json:"execution"` + Delivery WorkflowDelivery `json:"delivery"` + Verification WorkflowVerification `json:"verification"` + Requirements []string `json:"requirements"` + ResolvedAt time.Time `json:"resolved_at"` +} + +type WorkflowIdentity struct { + ID string `json:"id"` + Name string `json:"name"` + Revision int `json:"revision"` + Source string `json:"source"` + DefinitionHash string `json:"definition_hash"` +} + +type AgentProfileSnapshot struct { + ID string `json:"id"` + Name string `json:"name"` + Role string `json:"role"` + Revision int `json:"revision"` + Instructions string `json:"instructions"` +} + +type WorkflowTrigger struct { + Kind string `json:"kind"` + Origin RunOrigin `json:"origin"` + RequestID string `json:"request_id,omitempty"` + ReceiptID string `json:"receipt_id,omitempty"` + Actor string `json:"actor,omitempty"` + Repository string `json:"repository,omitempty"` + Object string `json:"object,omitempty"` + Action string `json:"action,omitempty"` + Ref string `json:"ref,omitempty"` + AutomationID string `json:"automation_id,omitempty"` + IdempotencyKey string `json:"idempotency_key"` + ConcurrencyGroup string `json:"concurrency_group"` +} + +type WorkflowExecution struct { + RunKind RunKind `json:"run_kind"` + LLMSelection WorkflowLLMSelection `json:"llm_selection"` + Session bool `json:"session"` + PermissionMode string `json:"permission_mode"` + WorkspaceAccess string `json:"workspace_access"` + ProviderCredentials string `json:"provider_credentials"` + BaseRef string `json:"base_ref,omitempty"` + TimeoutSeconds int64 `json:"timeout_seconds"` + TimeoutSource string `json:"timeout_source"` +} + +type WorkflowLLMSelection struct { + ModelID string `json:"model_id,omitempty"` + ModelName string `json:"model_name"` + Effort string `json:"effort,omitempty"` + Source string `json:"source"` +} + +type WorkflowDelivery struct { + Outputs []WorkflowOutput `json:"outputs"` + Merge string `json:"merge"` +} + +type WorkflowOutput struct { + Type string `json:"type"` + Target string `json:"target,omitempty"` + ReadyPolicy PRReadyPolicy `json:"ready_policy,omitempty"` +} + +type WorkflowVerification struct { + Mode string `json:"mode"` + RulesRevision string `json:"rules_revision,omitempty"` + MaxFindings int `json:"max_findings,omitempty"` + MinimumConfidence int `json:"minimum_confidence,omitempty"` + RequiredRecords []string `json:"required_records,omitempty"` +} + +// Delivers reports whether the immutable contract authorizes an output type. +// Runtime delivery code must consult this projection instead of re-reading a +// Service policy which may have changed after the Run was created. +func (c *WorkflowContract) Delivers(outputType string) bool { + if c == nil { + return false + } + for _, output := range c.Delivery.Outputs { + if output.Type == outputType { + return true + } + } + return false +} + +// DeliveryTarget returns the frozen target for an authorized output. +func (c *WorkflowContract) DeliveryTarget(outputType string) string { + if c == nil { + return "" + } + for _, output := range c.Delivery.Outputs { + if output.Type == outputType { + return output.Target + } + } + return "" +} + +// DeliveryReadyPolicy returns the policy frozen with a pull-request output. +func (c *WorkflowContract) DeliveryReadyPolicy(outputType string) PRReadyPolicy { + if c == nil { + return "" + } + for _, output := range c.Delivery.Outputs { + if output.Type == outputType { + return output.ReadyPolicy + } + } + return "" +} + +type builtinWorkflowDefinition struct { + ID string `json:"id"` + Name string `json:"name"` + Revision int `json:"revision"` + ProfileID string `json:"profile_id"` + Requirements []string `json:"requirements"` + Outputs []string `json:"outputs"` + Prompt string `json:"prompt"` +} + +var implementationDefinition = builtinWorkflowDefinition{ + ID: BuiltinImplementationTaskID, Name: "Implementation task", Revision: 1, + ProfileID: BuiltinDeveloperProfileID, + Requirements: []string{RequirementSourceRead, RequirementSourceWrite, RequirementGit, RequirementShell, RequirementRipgrep}, + Outputs: []string{OutputDiffOnly, OutputCreatePullRequest}, + Prompt: "Implement the requested change in the frozen repository context, verify it proportionately, and emit only the delivery outputs allowed by the contract.", +} + +var reviewDefinition = builtinWorkflowDefinition{ + ID: BuiltinPullRequestReviewID, Name: "Pull Request Review", Revision: 1, + ProfileID: BuiltinReviewerProfileID, + Requirements: []string{RequirementSourceRead, RequirementGit, RequirementRipgrep, RequirementSCMReviewWrite}, + Outputs: []string{OutputProviderReview}, + Prompt: "Review only the exact frozen revision pair and anchor every finding to a changed right-side line in the server-accepted review plan.", +} + +func ResolveWorkflowContract(run *Run, service *Service, timeoutSeconds int64, timeoutSource string, resolvedAt time.Time) (*WorkflowContract, error) { + if run == nil || service == nil { + return nil, errors.New("run and service are required") + } + if timeoutSeconds <= 0 { + return nil, errors.New("effective timeout must be positive") + } + if timeoutSource != TimeoutSourceProject && timeoutSource != TimeoutSourceCluster { + return nil, errors.New("invalid timeout source") + } + kind := run.Kind + if kind == "" { + kind = RunKindAgent + } + permissionMode := strings.TrimSpace(run.PermissionMode) + if permissionMode == "" { + permissionMode = "full_access" + } + baseRef := strings.TrimSpace(run.BaseBranch) + if baseRef == "" { + baseRef = strings.TrimSpace(service.DefaultBranch) + } + if kind == RunKindReview && run.PRBaseBranch != "" { + baseRef = run.PRBaseBranch + } + contract := &WorkflowContract{ + SchemaVersion: WorkflowContractSchemaVersion, + Trigger: resolveWorkflowTrigger(run, service), + Execution: WorkflowExecution{ + RunKind: kind, LLMSelection: WorkflowLLMSelection{ModelName: run.ModelName, Effort: run.ModelEffort, Source: "resolved_run"}, + Session: run.Session, PermissionMode: permissionMode, ProviderCredentials: "none", BaseRef: baseRef, + TimeoutSeconds: timeoutSeconds, TimeoutSource: timeoutSource, + }, + Delivery: WorkflowDelivery{Merge: "never"}, + ResolvedAt: resolvedAt.UTC(), + } + if run.ModelID != nil { + contract.Execution.LLMSelection.ModelID = *run.ModelID + } + if kind == RunKindReview { + contract.Workflow = workflowIdentity(reviewDefinition) + contract.Profile = AgentProfileSnapshot{ID: BuiltinReviewerProfileID, Name: "Reviewer", Role: "reviewer", Revision: 1, + Instructions: "Review the exact frozen change and report only validated, changed-line findings."} + contract.Execution.WorkspaceAccess = WorkspaceReadOnly + contract.Delivery.Outputs = []WorkflowOutput{{Type: OutputProviderReview, Target: "trigger_pr"}} + contract.Verification = WorkflowVerification{Mode: "structured_review", RulesRevision: "review-v2", MaxFindings: MaxReviewFindings, MinimumConfidence: 80} + contract.Requirements = append([]string(nil), reviewDefinition.Requirements...) + } else { + contract.Workflow = workflowIdentity(implementationDefinition) + contract.Profile = AgentProfileSnapshot{ID: BuiltinDeveloperProfileID, Name: "Developer", Role: "developer", Revision: 1, + Instructions: "Implement the request in the frozen repository context and verify the resulting change."} + contract.Execution.WorkspaceAccess = WorkspaceReadWrite + contract.Verification = WorkflowVerification{Mode: "runner_reported"} + contract.Requirements = append([]string(nil), implementationDefinition.Requirements...) + if service.GitMode == GitModeDraftPR && service.RepoKind == RepoKindProvider { + policy := run.PRReadyPolicy + if policy == "" { + policy = service.PRReadyPolicy + } + if policy == "" { + policy = PRReadyPolicyAlwaysDraft + } + target := "service_repository" + if run.Origin == RunOriginWebhook && run.PRNumber > 0 { + target = "trigger_pr" + } + contract.Delivery.Outputs = []WorkflowOutput{{Type: OutputCreatePullRequest, Target: target, ReadyPolicy: policy}} + contract.Requirements = append(contract.Requirements, RequirementSCMPullRequestWrite) + } else { + contract.Delivery.Outputs = []WorkflowOutput{{Type: OutputDiffOnly}} + } + } + hash, err := contract.CanonicalHash() + if err != nil { + return nil, err + } + contract.Hash = hash + if err := contract.Validate(); err != nil { + return nil, err + } + return contract, nil +} + +func workflowIdentity(def builtinWorkflowDefinition) WorkflowIdentity { + return WorkflowIdentity{ID: def.ID, Name: def.Name, Revision: def.Revision, Source: "builtin", DefinitionHash: hashJSON(def)} +} + +func resolveWorkflowTrigger(run *Run, service *Service) WorkflowTrigger { + kind := "api" + switch run.Origin { + case RunOriginKanban: + kind = "jtype" + case RunOriginSchedule: + kind = "cron" + case RunOriginAutomation, RunOriginWebhook: + kind = "scm" + case RunOriginAPI: + if run.TriggeredByUserID != nil { + kind = "manual" + } + } + actor := "" + if run.ProvenanceSnapshot.RequestedActor != nil { + actor = run.ProvenanceSnapshot.RequestedActor.Kind + ":" + run.ProvenanceSnapshot.RequestedActor.ID + } else if run.ProvenanceSnapshot.AccountableActor != nil { + actor = run.ProvenanceSnapshot.AccountableActor.Kind + ":" + run.ProvenanceSnapshot.AccountableActor.ID + } else if run.TriggeredByUserID != nil { + actor = "cloud_user:" + *run.TriggeredByUserID + } + repository := service.RepoOwnerName + if repository == "" { + repository = service.RawRepoURL + } + object := "" + if run.PRNumber > 0 { + object = fmt.Sprintf("pull_request:%d", run.PRNumber) + } + idempotency := run.OriginEventKey + if idempotency == "" { + idempotency = run.OriginCommentID + } + if idempotency == "" { + idempotency = run.ID + } + ref := run.BaseBranch + if run.PRHeadBranch != "" { + ref = run.PRHeadBranch + } + group := service.ID + if object != "" { + group += ":" + object + } + return WorkflowTrigger{Kind: kind, Origin: run.Origin, RequestID: run.ID, ReceiptID: firstNonEmpty(run.OriginEventKey, run.OriginCommentID), + Actor: actor, Repository: repository, Object: object, Ref: ref, AutomationID: run.OriginAutomationID, + IdempotencyKey: idempotency, ConcurrencyGroup: group} +} + +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} + +func (c WorkflowContract) CanonicalHash() (string, error) { + view := struct { + SchemaVersion int `json:"schema_version"` + Workflow WorkflowIdentity `json:"workflow"` + Profile AgentProfileSnapshot `json:"profile"` + Trigger WorkflowTrigger `json:"trigger"` + Execution WorkflowExecution `json:"execution"` + Delivery WorkflowDelivery `json:"delivery"` + Verification WorkflowVerification `json:"verification"` + Requirements []string `json:"requirements"` + }{c.SchemaVersion, c.Workflow, c.Profile, c.Trigger, c.Execution, c.Delivery, c.Verification, c.Requirements} + data, err := json.Marshal(view) + if err != nil { + return "", fmt.Errorf("encode workflow contract: %w", err) + } + return sha256String(data), nil +} + +func (c WorkflowContract) Validate() error { + if c.SchemaVersion != WorkflowContractSchemaVersion { + return errors.New("unsupported workflow contract schema") + } + if c.Execution.TimeoutSeconds <= 0 { + return errors.New("workflow timeout must be positive") + } + if c.Execution.TimeoutSource != TimeoutSourceProject && c.Execution.TimeoutSource != TimeoutSourceCluster { + return errors.New("invalid workflow timeout source") + } + if c.Execution.ProviderCredentials != "none" { + return errors.New("provider credentials may not enter the Runner") + } + if c.Delivery.Merge != "never" || len(c.Delivery.Outputs) != 1 { + return errors.New("workflow must have exactly one non-merge delivery output") + } + for _, req := range c.Requirements { + if !slices.Contains(validWorkflowRequirements, req) { + return fmt.Errorf("unknown workflow requirement %q", req) + } + } + var def builtinWorkflowDefinition + switch c.Workflow.ID { + case BuiltinImplementationTaskID: + def = implementationDefinition + if c.Workflow.Revision != 1 || c.Profile.ID != BuiltinDeveloperProfileID || c.Execution.RunKind != RunKindAgent || c.Execution.WorkspaceAccess != WorkspaceReadWrite { + return errors.New("invalid implementation workflow mapping") + } + out := c.Delivery.Outputs[0] + if out.Type != OutputDiffOnly && out.Type != OutputCreatePullRequest { + return errors.New("implementation workflow has an invalid output") + } + if out.Type == OutputCreatePullRequest && !ValidPRReadyPolicy(out.ReadyPolicy) { + return errors.New("pull-request output requires ready_policy") + } + if out.Type == OutputDiffOnly && out.ReadyPolicy != "" { + return errors.New("diff output forbids ready_policy") + } + case BuiltinPullRequestReviewID: + def = reviewDefinition + out := c.Delivery.Outputs[0] + if c.Workflow.Revision != 1 || c.Profile.ID != BuiltinReviewerProfileID || c.Execution.RunKind != RunKindReview || c.Execution.WorkspaceAccess != WorkspaceReadOnly || out.Type != OutputProviderReview || out.ReadyPolicy != "" { + return errors.New("invalid review workflow mapping") + } + if c.Verification.Mode != "structured_review" || c.Verification.RulesRevision != "review-v2" { + return errors.New("invalid review verification contract") + } + default: + return errors.New("unknown built-in workflow") + } + if c.Workflow.DefinitionHash != hashJSON(def) { + return errors.New("workflow definition hash mismatch") + } + want, err := c.CanonicalHash() + if err != nil { + return err + } + if c.Hash != want { + return errors.New("workflow contract hash mismatch") + } + return nil +} + +func hashJSON(value any) string { + data, _ := json.Marshal(value) + return sha256String(data) +} + +func sha256String(data []byte) string { + sum := sha256.Sum256(data) + return "sha256:" + hex.EncodeToString(sum[:]) +} diff --git a/orchestrator/internal/domain/workflow_contract_test.go b/orchestrator/internal/domain/workflow_contract_test.go new file mode 100644 index 00000000..41ef9448 --- /dev/null +++ b/orchestrator/internal/domain/workflow_contract_test.go @@ -0,0 +1,114 @@ +package domain + +import ( + "testing" + "time" +) + +func TestResolveWorkflowContractBuiltins(t *testing.T) { + now := time.Date(2026, 8, 1, 1, 2, 3, 0, time.UTC) + svc := Service{ID: "svc", ProjectID: "project", RepoKind: RepoKindProvider, Provider: ProviderGitHub, + RepoOwnerName: "cnjack/cloud", DefaultBranch: "main", GitMode: GitModeDraftPR, + PRReadyPolicy: PRReadyPolicyLifecycleAware} + tests := []struct { + name string + run Run + workflowID string + profileID string + output string + workspace string + requirement string + }{ + {name: "developer", run: Run{ID: "agent", Kind: RunKindAgent, Origin: RunOriginKanban, ModelName: "zhipu/glm-5.2"}, + workflowID: BuiltinImplementationTaskID, profileID: BuiltinDeveloperProfileID, + output: OutputCreatePullRequest, workspace: WorkspaceReadWrite, requirement: RequirementSourceWrite}, + {name: "reviewer", run: Run{ID: "review", Kind: RunKindReview, Origin: RunOriginAutomation, + ModelName: "zhipu/glm-5.2", PRNumber: 24, PRHeadBranch: "feature", PRBaseBranch: "main"}, + workflowID: BuiltinPullRequestReviewID, profileID: BuiltinReviewerProfileID, + output: OutputProviderReview, workspace: WorkspaceReadOnly, requirement: RequirementSCMReviewWrite}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + contract, err := ResolveWorkflowContract(&tc.run, &svc, 3600, TimeoutSourceProject, now) + if err != nil { + t.Fatal(err) + } + if contract.Workflow.ID != tc.workflowID || contract.Profile.ID != tc.profileID { + t.Fatalf("identity = %s/%s", contract.Workflow.ID, contract.Profile.ID) + } + if contract.Delivery.Outputs[0].Type != tc.output || contract.Execution.WorkspaceAccess != tc.workspace { + t.Fatalf("delivery/workspace = %s/%s", contract.Delivery.Outputs[0].Type, contract.Execution.WorkspaceAccess) + } + if !containsRequirement(contract.Requirements, tc.requirement) { + t.Fatalf("missing %s", tc.requirement) + } + if contract.Execution.TimeoutSeconds != 3600 || contract.Execution.TimeoutSource != TimeoutSourceProject { + t.Fatalf("timeout = %d/%s", contract.Execution.TimeoutSeconds, contract.Execution.TimeoutSource) + } + if err := contract.Validate(); err != nil { + t.Fatalf("validate: %v", err) + } + if contract.Hash == "" || contract.Workflow.DefinitionHash == "" { + t.Fatal("hashes were not resolved") + } + }) + } +} + +func TestWorkflowContractHashExcludesResolutionTimeAndDetectsTamper(t *testing.T) { + svc := Service{RepoKind: RepoKindRaw, RawRepoURL: "https://example.invalid/repo.git", DefaultBranch: "main", GitMode: GitModeReadonly} + run := Run{ID: "r", Kind: RunKindAgent, Origin: RunOriginAPI, ModelName: "zhipu/glm-5.2"} + a, err := ResolveWorkflowContract(&run, &svc, 43200, TimeoutSourceCluster, time.Unix(1, 0).UTC()) + if err != nil { + t.Fatal(err) + } + b, err := ResolveWorkflowContract(&run, &svc, 43200, TimeoutSourceCluster, time.Unix(2, 0).UTC()) + if err != nil { + t.Fatal(err) + } + if a.Hash != b.Hash { + t.Fatalf("resolved_at changed hash: %s != %s", a.Hash, b.Hash) + } + a.Execution.TimeoutSeconds++ + if err := a.Validate(); err == nil { + t.Fatal("tampered contract accepted") + } +} + +func TestWorkflowContractRejectsDeliveryConflict(t *testing.T) { + svc := Service{RepoKind: RepoKindProvider, Provider: ProviderGitHub, RepoOwnerName: "cnjack/cloud", DefaultBranch: "main", GitMode: GitModeDraftPR, PRReadyPolicy: PRReadyPolicyLifecycleAware} + run := Run{ID: "r", Kind: RunKindReview, Origin: RunOriginAPI, ModelName: "zhipu/glm-5.2"} + c, err := ResolveWorkflowContract(&run, &svc, 43200, TimeoutSourceCluster, time.Now().UTC()) + if err != nil { + t.Fatal(err) + } + c.Delivery.Outputs[0].Type = OutputCreatePullRequest + c.Hash, _ = c.CanonicalHash() + if err := c.Validate(); err == nil { + t.Fatal("review contract accepted pull-request delivery") + } +} + +func TestBuiltinWorkflowDefinitionHashesRequireExplicitRevisionBump(t *testing.T) { + // If a built-in definition intentionally changes, bump its Revision and then + // update this golden hash. This prevents a semantic edit from masquerading as + // the same code-owned Workflow version in already persisted Run contracts. + want := map[string]string{ + BuiltinImplementationTaskID: "sha256:3fafba335a9fea06aec3861c40f886d377f84b3198caf8540b1c8e3d599eae85", + BuiltinPullRequestReviewID: "sha256:8851abf6a4ba55cde7f37a8423ef93a1ff4e8659b6e010bc53c86030c53de62f", + } + for _, definition := range []builtinWorkflowDefinition{implementationDefinition, reviewDefinition} { + if got := workflowIdentity(definition).DefinitionHash; got != want[definition.ID] { + t.Fatalf("%s revision %d definition changed: got %s", definition.ID, definition.Revision, got) + } + } +} + +func containsRequirement(values []string, want string) bool { + for _, value := range values { + if value == want { + return true + } + } + return false +} diff --git a/orchestrator/internal/provider/fake.go b/orchestrator/internal/provider/fake.go index 3b155f67..eed7bd52 100644 --- a/orchestrator/internal/provider/fake.go +++ b/orchestrator/internal/provider/fake.go @@ -185,7 +185,8 @@ func (f *FakeProvider) PRByNumber(_ context.Context, owner, repo string, prNumbe return &cp, nil } return &PR{Number: prNumber, URL: fmt.Sprintf("http://gitea.test/%s/%s/pulls/%d", owner, repo, prNumber), - State: "open", HeadRef: fmt.Sprintf("pr-%d-head", prNumber), BaseRef: "main"}, nil + State: "open", HeadRef: fmt.Sprintf("pr-%d-head", prNumber), BaseRef: "main", + HeadSHA: fmt.Sprintf("%040x", prNumber+1), BaseSHA: fmt.Sprintf("%040x", prNumber)}, nil } // CreateIssueComment records a comment (or returns the injected error). diff --git a/orchestrator/internal/provider/gitea.go b/orchestrator/internal/provider/gitea.go index ea7d6ace..fd34d950 100644 --- a/orchestrator/internal/provider/gitea.go +++ b/orchestrator/internal/provider/gitea.go @@ -71,9 +71,11 @@ type giteaPR struct { Title string `json:"title"` Head struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"head"` Base struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"base"` } @@ -176,6 +178,8 @@ func (c *GiteaClient) PRByNumber(ctx context.Context, owner, repo string, prNumb Draft: strings.HasPrefix(pr.Title, GiteaWIPPrefix), HeadRef: pr.Head.Ref, BaseRef: pr.Base.Ref, + HeadSHA: pr.Head.SHA, + BaseSHA: pr.Base.SHA, }, nil } diff --git a/orchestrator/internal/provider/github.go b/orchestrator/internal/provider/github.go index 4126969b..56268e85 100644 --- a/orchestrator/internal/provider/github.go +++ b/orchestrator/internal/provider/github.go @@ -44,9 +44,11 @@ type githubPR struct { NodeID string `json:"node_id"` Head struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"head"` Base struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"base"` } @@ -164,7 +166,7 @@ func (c *GitHubClient) PRByNumber(ctx context.Context, owner, repo string, prNum return nil, err } return &PR{Number: pr.Number, URL: pr.HTMLURL, State: prState(pr.State, pr.Merged), Draft: pr.Draft, - HeadRef: pr.Head.Ref, BaseRef: pr.Base.Ref}, nil + HeadRef: pr.Head.Ref, BaseRef: pr.Base.Ref, HeadSHA: pr.Head.SHA, BaseSHA: pr.Base.SHA}, nil } func (c *GitHubClient) CreateIssueComment(ctx context.Context, owner, repo string, issueNumber int, body string) error { diff --git a/orchestrator/internal/provider/gitlab.go b/orchestrator/internal/provider/gitlab.go index 029a1827..5e9f7c82 100644 --- a/orchestrator/internal/provider/gitlab.go +++ b/orchestrator/internal/provider/gitlab.go @@ -69,6 +69,10 @@ type gitlabMR struct { Draft bool `json:"draft"` SourceBranch string `json:"source_branch"` TargetBranch string `json:"target_branch"` + DiffRefs struct { + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + } `json:"diff_refs"` } func (c *GitLabClient) auth() string { return "Bearer " + c.token } @@ -180,7 +184,7 @@ func (c *GitLabClient) PRByNumber(ctx context.Context, owner, repo string, prNum return nil, err } return &PR{Number: mr.IID, URL: mr.WebURL, State: gitlabState(mr.State), Draft: gitLabMRDraft(mr), - HeadRef: mr.SourceBranch, BaseRef: mr.TargetBranch}, nil + HeadRef: mr.SourceBranch, BaseRef: mr.TargetBranch, HeadSHA: mr.DiffRefs.HeadSHA, BaseSHA: mr.DiffRefs.BaseSHA}, nil } func (c *GitLabClient) CreateIssueComment(ctx context.Context, owner, repo string, issueNumber int, body string) error { diff --git a/orchestrator/internal/provider/provider.go b/orchestrator/internal/provider/provider.go index 090190e1..4da590d6 100644 --- a/orchestrator/internal/provider/provider.go +++ b/orchestrator/internal/provider/provider.go @@ -27,6 +27,8 @@ type PR struct { // not carry them. HeadRef string BaseRef string + HeadSHA string + BaseSHA string } // CreateDraftPRInput is the request to open a draft PR. diff --git a/orchestrator/internal/reconciler/decision.go b/orchestrator/internal/reconciler/decision.go index 796bc946..24b351e8 100644 --- a/orchestrator/internal/reconciler/decision.go +++ b/orchestrator/internal/reconciler/decision.go @@ -174,14 +174,17 @@ func shouldOpenPR(run domain.Run, svc domain.Service, providerReady bool) bool { if run.Status != domain.StatusSucceeded || run.Kind == domain.RunKindReview { return false } - if svc.GitMode != domain.GitModeDraftPR { - return false - } - if svc.RepoKind != domain.RepoKindProvider || !domain.ValidProvider(svc.Provider) { - return false - } - if strings.TrimSpace(svc.RepoOwnerName) == "" { - return false + if run.ExecutionContract != nil { + if !run.ExecutionContract.Delivers(domain.OutputCreatePullRequest) || + run.ExecutionContract.DeliveryTarget(domain.OutputCreatePullRequest) != "service_repository" { + return false + } + } else { + // Rolling-upgrade behavior for historical Runs without a contract. + if svc.GitMode != domain.GitModeDraftPR || svc.RepoKind != domain.RepoKindProvider || + !domain.ValidProvider(svc.Provider) || strings.TrimSpace(svc.RepoOwnerName) == "" { + return false + } } if strings.TrimSpace(run.GitBranch) == "" { return false // no bundle received yet (branch not recorded) @@ -211,11 +214,17 @@ func shouldUpdatePush(run domain.Run, svc domain.Service, providerReady bool) bo if run.Origin != domain.RunOriginWebhook { return false } - if svc.GitMode != domain.GitModeDraftPR || svc.RepoKind != domain.RepoKindProvider || !domain.ValidProvider(svc.Provider) { - return false - } - if strings.TrimSpace(svc.RepoOwnerName) == "" { - return false + if run.ExecutionContract != nil { + if !run.ExecutionContract.Delivers(domain.OutputCreatePullRequest) || + run.ExecutionContract.DeliveryTarget(domain.OutputCreatePullRequest) != "trigger_pr" { + return false + } + } else { + // Rolling-upgrade behavior for historical Runs without a contract. + if svc.GitMode != domain.GitModeDraftPR || svc.RepoKind != domain.RepoKindProvider || + !domain.ValidProvider(svc.Provider) || strings.TrimSpace(svc.RepoOwnerName) == "" { + return false + } } if strings.TrimSpace(run.GitBranch) == "" || run.PRURL == "" { return false // no bundle received, or no target PR diff --git a/orchestrator/internal/reconciler/guardrails_test.go b/orchestrator/internal/reconciler/guardrails_test.go index 4ce0c8c0..5cc76142 100644 --- a/orchestrator/internal/reconciler/guardrails_test.go +++ b/orchestrator/internal/reconciler/guardrails_test.go @@ -217,6 +217,7 @@ func TestRunTimeoutClusterDefault(t *testing.T) { ctx := context.Background() rec, st, fake := testRec(t, 10) rec.cfg.RunTimeoutSecs = 43200 + store.ConfigureWorkflowTimeoutDefaults(st, rec.cfg.RunTimeoutSecs, rec.cfg.SessionTTLSecs) _, _ = seedGuardrailProject(t, st, &domain.Project{RunTimeoutSecs: nil}, 1) rec.Tick(ctx) diff --git a/orchestrator/internal/reconciler/permission_test.go b/orchestrator/internal/reconciler/permission_test.go index c69658d8..cd5b58ca 100644 --- a/orchestrator/internal/reconciler/permission_test.go +++ b/orchestrator/internal/reconciler/permission_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/cnjack/jcloud/internal/domain" + "github.com/cnjack/jcloud/internal/store" ) // F8b reconciler half: a permission_mode=approval SESSION run gets @@ -35,6 +36,7 @@ func TestApprovalSessionJobEnv(t *testing.T) { ctx := context.Background() rec, st, fake := testRec(t, 10) rec.cfg.SessionTTLSecs = 7200 + store.ConfigureWorkflowTimeoutDefaults(st, rec.cfg.RunTimeoutSecs, rec.cfg.SessionTTLSecs) pid, sid := seedSessionProject(t, st, &domain.Project{}) queueApprovalSession(t, st, pid, sid) @@ -62,6 +64,7 @@ func TestApprovalSessionTimeoutScalesWithSmallTTL(t *testing.T) { ctx := context.Background() rec, st, fake := testRec(t, 10) rec.cfg.SessionTTLSecs = 7200 // cluster default would cap at 300… + store.ConfigureWorkflowTimeoutDefaults(st, rec.cfg.RunTimeoutSecs, rec.cfg.SessionTTLSecs) ttl := int64(400) pid, sid := seedSessionProject(t, st, &domain.Project{SessionTTLSecs: &ttl}) // …project override wins queueApprovalSession(t, st, pid, sid) diff --git a/orchestrator/internal/reconciler/pr_test.go b/orchestrator/internal/reconciler/pr_test.go index a6818a0a..fd7dfae7 100644 --- a/orchestrator/internal/reconciler/pr_test.go +++ b/orchestrator/internal/reconciler/pr_test.go @@ -123,6 +123,80 @@ func seedDraftPRRun(t *testing.T, st *store.MemStore, branch string) (domain.Ser return *svc, *got } +func TestSCMContextKeepsClaimedRepositoryAndCredentialAfterReconnect(t *testing.T) { + ctx := context.Background() + rec, st, _ := testRec(t, 4) + cipher := prTestCipher(t) + oldToken, _ := cipher.EncryptString("old-bot-token") + newToken, _ := cipher.EncryptString("new-bot-token") + svc, run := seedDraftPRRun(t, st, "jcode/run-frozen") + + oldCfg := &domain.ProviderConfig{Provider: domain.PluginGitea, BaseURL: "https://old-gitea.example", PluginEnabled: true} + if err := st.UpsertProviderConfig(ctx, oldCfg); err != nil { + t.Fatal(err) + } + installation := &domain.PluginInstallation{ + ID: domain.NewID(), ProjectID: svc.ProjectID, Provider: domain.PluginGitea, + Status: domain.PluginStatusEnabled, AccessTokenEnc: oldToken, + ConfigRevision: oldCfg.ConfigRevision, CreatedAt: time.Now().UTC(), + } + if err := st.CreatePluginInstallation(ctx, installation); err != nil { + t.Fatal(err) + } + oldBinding := &domain.ServiceRepositoryBinding{ + ServiceID: svc.ID, InstallationID: installation.ID, ProviderRepoID: "repo-42", + RepositoryPath: "acme/old-name", CloneURL: "https://old-gitea.example/acme/old-name.git", DefaultBranch: "main", + } + if err := st.UpsertServiceRepositoryBinding(ctx, oldBinding); err != nil { + t.Fatal(err) + } + candidates, err := rec.runPluginSnapshotCandidates(ctx, &run) + if err != nil { + t.Fatal(err) + } + if err := st.CreateRunPluginSnapshots(ctx, candidates); err != nil { + t.Fatal(err) + } + + // A reconnect plus repository rename changes all live records after claim. + newCfg := &domain.ProviderConfig{Provider: domain.PluginGitea, BaseURL: "https://new-gitea.example", PluginEnabled: true} + if err := st.UpsertProviderConfigAndInvalidate(ctx, newCfg, true, "reconnect"); err != nil { + t.Fatal(err) + } + live, _ := st.GetPluginInstallation(ctx, installation.ID) + live.Status, live.ConfigRevision, live.AccessTokenEnc = domain.PluginStatusEnabled, newCfg.ConfigRevision, newToken + if err := st.UpdatePluginInstallation(ctx, live); err != nil { + t.Fatal(err) + } + if err := st.UpsertServiceRepositoryBinding(ctx, &domain.ServiceRepositoryBinding{ + ServiceID: svc.ID, InstallationID: installation.ID, ProviderRepoID: "repo-42", + RepositoryPath: "acme/new-name", CloneURL: "https://new-gitea.example/acme/new-name.git", DefaultBranch: "trunk", + }); err != nil { + t.Fatal(err) + } + svc.RepoOwnerName, svc.DefaultBranch = "acme/new-name", "trunk" + if err := st.UpdateService(ctx, &svc); err != nil { + t.Fatal(err) + } + + rec.WithPRStack(&fakeFactory{p: provider.NewFakeProvider()}, &fakePusher{}, credentials.NewResolver(st, cipher, nil, "", nil)) + got, err := rec.scmContextForRun(ctx, &run, &svc) + if err != nil { + t.Fatal(err) + } + if !got.Frozen || got.RepositoryPath != "acme/old-name" || got.CloneURL != "https://old-gitea.example/acme/old-name.git" || got.DefaultBranch != "main" { + t.Fatalf("claimed repository grant drifted: %+v", got) + } + if got.Token.Value != "old-bot-token" || got.Token.Source != "plugin_snapshot" { + t.Fatalf("claimed credential drifted: source=%q value=%q", got.Token.Source, got.Token.Value) + } + env := map[string]string{} + rec.addGitEnv(ctx, env, &run, &svc) + if env["REPO_URL"] != oldBinding.CloneURL || env["BASE_BRANCH"] != oldBinding.DefaultBranch { + t.Fatalf("runner source drifted after claim: REPO_URL=%q BASE_BRANCH=%q", env["REPO_URL"], env["BASE_BRANCH"]) + } +} + // TestReconcilePRCreation covers the happy path: a succeeded draft_pr run with a // stored bundle gets its branch pushed and exactly one draft PR opened, pr_url / // pr_number persisted, and a run.status event carrying pr_url emitted. @@ -400,7 +474,8 @@ func seedReviewRun(t *testing.T, st *store.MemStore, head, output string) domain run := &domain.Run{ ID: domain.NewID(), ProjectID: p.ID, ServiceID: svc.ID, Prompt: "review the PR", Status: domain.StatusSucceeded, Kind: domain.RunKindReview, - PRHeadBranch: head, PRBaseBranch: "main", CreatedAt: time.Now(), + PRHeadBranch: head, PRBaseBranch: "main", + PRHeadSHA: strings.Repeat("a", 40), PRBaseSHA: strings.Repeat("b", 40), CreatedAt: time.Now(), } _ = st.CreateRun(ctx, run) if _, err := st.SetReviewOutput(ctx, run.ID, output); err != nil { @@ -766,6 +841,30 @@ func TestShouldUpdatePush(t *testing.T) { } } +func TestDeliveryGatesUseFrozenContractAfterServiceModeChanges(t *testing.T) { + createPR := &domain.WorkflowContract{Delivery: domain.WorkflowDelivery{Outputs: []domain.WorkflowOutput{{Type: domain.OutputCreatePullRequest, Target: "service_repository"}}}} + diffOnly := &domain.WorkflowContract{Delivery: domain.WorkflowDelivery{Outputs: []domain.WorkflowOutput{{Type: domain.OutputDiffOnly}}}} + svc := domain.Service{RepoKind: domain.RepoKindProvider, Provider: domain.ProviderGitea, RepoOwnerName: "o/r", GitMode: domain.GitModeReadonly} + run := domain.Run{Status: domain.StatusSucceeded, Kind: domain.RunKindAgent, GitBranch: "jcode/run-x", ExecutionContract: createPR} + if !shouldOpenPR(run, svc, true) { + t.Fatal("live readonly mutation overrode the frozen create_pull_request delivery") + } + svc.GitMode = domain.GitModeDraftPR + run.ExecutionContract = diffOnly + if shouldOpenPR(run, svc, true) { + t.Fatal("live draft_pr mutation overrode the frozen diff_only delivery") + } + + update := run + update.Origin = domain.RunOriginWebhook + update.PRURL = "https://example.test/pulls/1" + update.ExecutionContract = &domain.WorkflowContract{Delivery: domain.WorkflowDelivery{Outputs: []domain.WorkflowOutput{{Type: domain.OutputCreatePullRequest, Target: "trigger_pr"}}}} + svc.GitMode = domain.GitModeReadonly + if !shouldUpdatePush(update, svc, true) { + t.Fatal("live readonly mutation overrode the frozen trigger_pr delivery") + } +} + // --- gates ------------------------------------------------------------------ // TestShouldOpenPR is the exhaustive table for the pure PR-creation gate. @@ -951,7 +1050,8 @@ func TestReconcileReviewParksOnIntegrationCredential(t *testing.T) { review := &domain.Run{ ID: domain.NewID(), ProjectID: svc.ProjectID, ServiceID: svc.ID, Prompt: "review", Status: domain.StatusSucceeded, Kind: domain.RunKindReview, - PRHeadBranch: "jcode/run-rvpark", PRBaseBranch: "main", Attempt: 1, CreatedAt: time.Now(), + PRHeadBranch: "jcode/run-rvpark", PRBaseBranch: "main", + PRHeadSHA: strings.Repeat("a", 40), PRBaseSHA: strings.Repeat("b", 40), Attempt: 1, CreatedAt: time.Now(), } if err := st.CreateRun(ctx, review); err != nil { t.Fatal(err) diff --git a/orchestrator/internal/reconciler/reconciler.go b/orchestrator/internal/reconciler/reconciler.go index 3cf68e37..e593432b 100644 --- a/orchestrator/internal/reconciler/reconciler.go +++ b/orchestrator/internal/reconciler/reconciler.go @@ -168,6 +168,10 @@ type KanbanWriter interface { // New builds a Reconciler. pub may be nil. The draft-PR / review stack is set // separately via WithPRStack so existing callers/tests are unaffected. func New(st store.Store, launcher k8s.JobLauncher, cfg *config.Config, log *slog.Logger, pub Publisher) *Reconciler { + // Run execution contracts are resolved at creation, not at scheduling. Wire + // the same cluster defaults into the Store here as a safe constructor-level + // invariant (main also does this immediately after migrations). + store.ConfigureWorkflowTimeoutDefaults(st, cfg.RunTimeoutSecs, cfg.SessionTTLSecs) return &Reconciler{ st: st, launcher: launcher, @@ -710,31 +714,22 @@ func (r *Reconciler) openPullRequest(ctx context.Context, run *domain.Run, svc * if r.integrationCredentialParked(run.ID) { return // parked (P1): credential problem already surfaced once } - owner, repo, ok := provider.SplitRepo(svc.RepoOwnerName) - if !ok { - r.log.Warn("reconcile pr: bad repo_owner_name", "run", run.ID, "repo", svc.RepoOwnerName) - return - } - branch := run.GitBranch // recorded when the bundle was received (jcode/run-) - - // Resolve the credential to act with: the service's integration bot token when - // bound (D19 / F5), else user OAuth / gitea PAT. The token value is never - // logged — only its source label. - tok, err := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) + scm, err := r.scmContextForRun(ctx, run, svc) if err != nil { - if errors.Is(err, credentials.ErrIntegrationCredential) { - // Persistent config error (P1): surface once + park, never spin. - r.noteIntegrationCredentialFailure(ctx, run.ID, "draft PR", err) + if errors.Is(err, credentials.ErrIntegrationCredential) || errors.Is(err, credentials.ErrPluginCredentialUnavailable) { + r.noteIntegrationCredentialFailure(ctx, run.ID, "pull request delivery", err) return } - r.log.Warn("reconcile pr: no credential; leaving diff-only", "run", run.ID, "provider", svc.Provider, "err", err) - return // retry next tick (a user may bind the provider later) + r.log.Warn("reconcile pr: resolve frozen scm context", "run", run.ID, "err", err) + return } - prov, err := r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) - if err != nil { - r.log.Warn("reconcile pr: build client", "run", run.ID, "provider", svc.Provider, "err", err) + owner, repo, ok := provider.SplitRepo(scm.RepositoryPath) + if !ok { + r.log.Warn("reconcile pr: bad repository path", "run", run.ID, "repo", scm.RepositoryPath) return } + branch := run.GitBranch // recorded when the bundle was received (jcode/run-) + tok, prov := scm.Token, scm.Client // Idempotency: an existing open PR for this head branch wins (covers a crash // after push/create but before persist, and a manually opened PR). @@ -752,7 +747,7 @@ func (r *Reconciler) openPullRequest(ctx context.Context, run *domain.Run, svc * // and closes the crash-recovery gap where merely finding the PR could // otherwise expose stale code as delivered. PushBundleBranch is non-forcing // and accepts an already-up-to-date branch. - sha, perr := r.pushRunBundle(ctx, run, svc, tok, branch) + sha, perr := r.pushRunBundle(ctx, run, scm.CloneURL, scm.Provider, tok, branch) if perr != nil { r.log.Warn("reconcile pr: ensure branch", "run", run.ID, "src", tok.Source, "err", perr) return // transient/non-fast-forward; retry without recording delivery @@ -794,9 +789,9 @@ func (r *Reconciler) openPullRequest(ctx context.Context, run *domain.Run, svc * } if pr == nil { // The branch is confirmed before the provider can expose the PR. - draft := run.PRReadyPolicy != domain.PRReadyPolicyLifecycleAware + draft := effectivePRReadyPolicy(run) != domain.PRReadyPolicyLifecycleAware pr, err = prov.CreatePR(ctx, provider.CreatePRInput{ - Owner: owner, Repo: repo, Head: branch, Base: svc.DefaultBranch, + Owner: owner, Repo: repo, Head: branch, Base: scm.DefaultBranch, Title: prTitle(run.Prompt), Body: prBody(run, r.prTriggerAttribution(ctx, run, svc)), Draft: draft, }) if err != nil { @@ -832,6 +827,18 @@ func (r *Reconciler) openPullRequest(ctx context.Context, run *domain.Run, svc * r.emitStatus(ctx, committed) } +func effectivePRReadyPolicy(run *domain.Run) domain.PRReadyPolicy { + if run != nil && run.ExecutionContract != nil { + if policy := run.ExecutionContract.DeliveryReadyPolicy(domain.OutputCreatePullRequest); policy != "" { + return policy + } + } + if run != nil { + return run.PRReadyPolicy + } + return "" +} + // openDraftPR is kept as an internal compatibility shim for focused legacy // tests/callers. The service policy still decides Draft versus Ready. func (r *Reconciler) openDraftPR(ctx context.Context, run *domain.Run, svc *domain.Service) { @@ -841,7 +848,7 @@ func (r *Reconciler) openDraftPR(ctx context.Context, run *domain.Run, svc *doma // pushRunBundle writes the run's stored bundle to a temp file and pushes its // branch to the provider using an authenticated clone/push URL. The temp file // (and the URL's credential) never persist. Returns the pushed branch tip SHA. -func (r *Reconciler) pushRunBundle(ctx context.Context, run *domain.Run, svc *domain.Service, tok credentials.Token, branch string) (string, error) { +func (r *Reconciler) pushRunBundle(ctx context.Context, run *domain.Run, cloneURL string, gitProvider domain.GitProvider, tok credentials.Token, branch string) (string, error) { bundle, err := r.st.GetRunBundle(ctx, run.ID) if err != nil { return "", fmt.Errorf("load bundle: %w", err) @@ -857,11 +864,10 @@ func (r *Reconciler) pushRunBundle(ctx context.Context, run *domain.Run, svc *do } f.Close() - rawURL := r.serviceCloneURL(ctx, svc) - if rawURL == "" { - return "", fmt.Errorf("could not derive push URL for service %s", svc.ID) + if cloneURL == "" { + return "", fmt.Errorf("could not derive push URL for run %s", run.ID) } - authed := tok.AuthedURL(rawURL, svc.Provider) + authed := tok.AuthedURL(cloneURL, gitProvider) return r.pusher.PushBundleBranch(ctx, authed, f.Name(), branch) } @@ -904,15 +910,16 @@ func (r *Reconciler) updatePushRun(ctx context.Context, run *domain.Run, svc *do return // parked (P1): credential problem already surfaced once } branch := run.GitBranch // recorded when the bundle was received (= PR head branch) - tok, err := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) + scm, err := r.scmContextForRun(ctx, run, svc) if err != nil { - if errors.Is(err, credentials.ErrIntegrationCredential) { + if errors.Is(err, credentials.ErrIntegrationCredential) || errors.Is(err, credentials.ErrPluginCredentialUnavailable) { r.noteIntegrationCredentialFailure(ctx, run.ID, "update push", err) return } - r.log.Warn("reconcile update: no credential; leaving for retry", "run", run.ID, "provider", svc.Provider, "err", err) + r.log.Warn("reconcile update: no frozen scm context; leaving for retry", "run", run.ID, "provider", svc.Provider, "err", err) return } + tok := scm.Token bundle, err := r.st.GetRunBundle(ctx, run.ID) if err != nil { @@ -932,12 +939,11 @@ func (r *Reconciler) updatePushRun(ctx context.Context, run *domain.Run, svc *do } f.Close() - rawURL := r.serviceCloneURL(ctx, svc) - if rawURL == "" { + if scm.CloneURL == "" { r.log.Warn("reconcile update: could not derive push URL", "run", run.ID) return } - authed := tok.AuthedURL(rawURL, svc.Provider) + authed := tok.AuthedURL(scm.CloneURL, scm.Provider) sha, alreadyPresent, err := r.pusher.PushBundleBranchFFOnly(ctx, authed, f.Name(), branch) if err != nil { @@ -1006,33 +1012,21 @@ func (r *Reconciler) postReview(ctx context.Context, run *domain.Run, svc *domai if svc.RepoKind != domain.RepoKindProvider || strings.TrimSpace(run.PRHeadBranch) == "" { return // not associated with a provider PR (misconfigured review run) } - owner, repo, ok := provider.SplitRepo(svc.RepoOwnerName) - if !ok { - r.log.Warn("reconcile review: bad repo_owner_name", "run", run.ID) - return - } - prov, credentialSource, usedPlugin, err := r.pluginReviewClient(ctx, run, svc) + scm, err := r.scmContextForRun(ctx, run, svc) if err != nil { - r.log.Warn("reconcile review: Plugin credential", "run", run.ID, "provider", svc.Provider, "err", err) - return - } - if !usedPlugin { - tok, resolveErr := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) - if resolveErr != nil { - if errors.Is(resolveErr, credentials.ErrIntegrationCredential) { - r.noteIntegrationCredentialFailure(ctx, run.ID, "review post", resolveErr) - return - } - r.log.Warn("reconcile review: no credential", "run", run.ID, "provider", svc.Provider, "err", resolveErr) - return - } - prov, err = r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) - credentialSource = tok.Source - if err != nil { - r.log.Warn("reconcile review: build client", "run", run.ID, "err", err) + if errors.Is(err, credentials.ErrIntegrationCredential) || errors.Is(err, credentials.ErrPluginCredentialUnavailable) { + r.noteIntegrationCredentialFailure(ctx, run.ID, "review post", err) return } + r.log.Warn("reconcile review: frozen scm credential", "run", run.ID, "provider", svc.Provider, "err", err) + return + } + owner, repo, ok := provider.SplitRepo(scm.RepositoryPath) + if !ok { + r.log.Warn("reconcile review: bad frozen repository path", "run", run.ID) + return } + prov, credentialSource := scm.Client, scm.Token.Source prNumber := run.PRNumber if prNumber <= 0 { pr, findErr := prov.FindOpenPRByHead(ctx, owner, repo, run.PRHeadBranch) @@ -1057,33 +1051,62 @@ func (r *Reconciler) postReview(ctx context.Context, run *domain.Run, svc *domai r.log.Info("reconcile review: posted review comment", "run", run.ID, "pr", prNumber, "src", credentialSource) } -func (r *Reconciler) pluginReviewClient(ctx context.Context, run *domain.Run, svc *domain.Service) (provider.Provider, string, bool, error) { - if run.OriginAutomationID == "" || r.creds == nil { - return nil, "", false, nil - } - binding, err := r.st.GetServiceRepositoryBinding(ctx, svc.ID) - if err != nil { - return nil, "", true, err +type runSCMContext struct { + Client provider.Provider + Token credentials.Token + Provider domain.GitProvider + RepositoryPath string + CloneURL string + DefaultBranch string + Frozen bool +} + +// scmContextForRun converges every post-claim SCM consumer on the same grant. +// New plugin-backed Runs use only their append-only credential/config and +// repository snapshot; reconnects, repository renames, and binding changes do +// not reinterpret an in-flight Run. Rows created before this contract retain a +// deliberate legacy fallback. +func (r *Reconciler) scmContextForRun(ctx context.Context, run *domain.Run, svc *domain.Service) (*runSCMContext, error) { + if r.creds == nil || r.factory == nil { + return nil, errors.New("scm stack is unavailable") } snapshots, err := r.st.ListRunPluginSnapshots(ctx, run.ID) if err != nil { - return nil, "", true, err + return nil, err } for i := range snapshots { snapshot := &snapshots[i] - if snapshot.InstallationID != binding.InstallationID { + if !snapshot.HasFrozenRepositoryGrant() { continue } credential, issueErr := r.creds.IssueRunPluginSnapshotCredential(ctx, snapshot) if issueErr != nil { - return nil, "", true, issueErr + return nil, issueErr } client, clientErr := provider.IntegrationClientWithScheme( - svc.Provider, credential.BaseURL, credential.AccessToken, credential.Scheme, + domain.GitProvider(snapshot.Provider), credential.BaseURL, credential.AccessToken, credential.Scheme, ) - return client, "plugin_snapshot", true, clientErr + if clientErr != nil { + return nil, clientErr + } + return &runSCMContext{ + Client: client, Token: credentials.Token{Value: credential.AccessToken, Scheme: credential.Scheme, Source: "plugin_snapshot"}, + Provider: domain.GitProvider(snapshot.Provider), RepositoryPath: snapshot.RepositoryPath, + CloneURL: snapshot.CloneURL, DefaultBranch: snapshot.DefaultBranch, Frozen: true, + }, nil } - return nil, "", true, errors.New("review Run has no matching Plugin credential snapshot") + + // Rolling-upgrade path for runs which predate repository grants. + tok, err := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) + if err != nil { + return nil, err + } + client, err := r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) + if err != nil { + return nil, err + } + return &runSCMContext{Client: client, Token: tok, Provider: svc.Provider, + RepositoryPath: svc.RepoOwnerName, CloneURL: r.serviceCloneURL(ctx, svc), DefaultBranch: svc.DefaultBranch}, nil } func postProviderReview(ctx context.Context, prov provider.Provider, owner, repo string, prNumber int, run *domain.Run) error { @@ -1536,6 +1559,15 @@ func (r *Reconciler) createJob(ctx context.Context, run *domain.Run, proj *domai r.log.Error("reconcile: createJob without a loaded project — refusing to schedule blind", "run", run.ID) return false } + if run.Kind == domain.RunKindReview && (strings.TrimSpace(run.PRBaseSHA) == "" || strings.TrimSpace(run.PRHeadSHA) == "") { + msg := "review_revision_unavailable: review requires a frozen base/head commit pair before the Runner starts" + if committed, err := r.st.MarkFailed(ctx, run.ID, "Failed", domain.FailureSetupFailed, msg, r.now()); err != nil { + r.log.Error("reconcile: mark review revision failure", "run", run.ID, "err", err) + } else { + r.emitStatus(ctx, committed) + } + return false + } // Fail-visible gate (CLAUDE.md red line #1): resolve the EFFECTIVE model // config at Job-launch time. The API gate blocks creation, but a run can be // queued while configured and then have its config cleared before it is @@ -1722,6 +1754,13 @@ func (r *Reconciler) createJob(ctx context.Context, run *domain.Run, proj *domai timeout = *proj.SessionTTLSecs } } + // New Runs carry the effective timeout frozen at creation. Mutable Project + // guardrails cannot reinterpret an already-visible execution contract while + // it waits in the queue. Legacy rows without a contract retain the historical + // calculation above during a rolling upgrade. + if run.ExecutionContract != nil && run.ExecutionContract.Execution.TimeoutSeconds > 0 { + timeout = run.ExecutionContract.Execution.TimeoutSeconds + } // The Job's activeDeadlineSeconds is a HARD backstop and counts from pod start // (including clone/setup), whereas RUN_TIMEOUT only bounds the agent turn. If we // set them equal, k8s would SIGKILL the pod at the same instant the runner's own @@ -2289,12 +2328,25 @@ func (r *Reconciler) runPluginSnapshotCandidates(ctx context.Context, run *domai return nil, err } snapshots := make([]domain.RunPluginSnapshot, 0, len(installations)) + binding, bindingErr := r.st.GetServiceRepositoryBinding(ctx, run.ServiceID) + if bindingErr != nil && !errors.Is(bindingErr, store.ErrNotFound) { + return nil, bindingErr + } for i := range installations { if r.pluginInstallationEligibleNow(ctx, &installations[i]) { - snapshots = append(snapshots, domain.RunPluginSnapshot{ + snapshot := domain.RunPluginSnapshot{ RunID: run.ID, InstallationID: installations[i].ID, Provider: installations[i].Provider, CreatedAt: r.now(), - }) + } + if bindingErr == nil && binding.InstallationID == installations[i].ID { + snapshot.RepositoryID = binding.ProviderRepoID + snapshot.RepositoryPath = binding.RepositoryPath + snapshot.CloneURL = binding.CloneURL + snapshot.DefaultBranch = binding.DefaultBranch + snapshot.ActingPrincipalKind = "provider_bot" + snapshot.ActingPrincipalID = installations[i].ID + } + snapshots = append(snapshots, snapshot) } } return snapshots, nil @@ -2429,7 +2481,20 @@ func (r *Reconciler) applyInjectedEnv(env map[string]string, run *domain.Run, pr // from the tmpfs credential helper written by the sidecar. func (r *Reconciler) addGitEnv(ctx context.Context, env map[string]string, run *domain.Run, svc *domain.Service) { isAgent := run.Kind == "" || run.Kind == domain.RunKindAgent - env["BASE_BRANCH"] = svc.DefaultBranch + defaultBranch := svc.DefaultBranch + repoURL := r.serviceCloneURL(ctx, svc) + // ClaimRunDispatch has already committed the repository grant before jobEnv + // runs. Prefer it over the mutable Service/binding so a rename in this narrow + // window cannot send the pod to a different repository or default branch. + if snapshots, err := r.st.ListRunPluginSnapshots(ctx, run.ID); err == nil { + for i := range snapshots { + if snapshots[i].HasFrozenRepositoryGrant() { + defaultBranch, repoURL = snapshots[i].DefaultBranch, snapshots[i].CloneURL + break + } + } + } + env["BASE_BRANCH"] = defaultBranch if run.BaseBranch != "" { env["BASE_BRANCH"] = run.BaseBranch } @@ -2440,10 +2505,17 @@ func (r *Reconciler) addGitEnv(ctx context.Context, env map[string]string, run * } env["SOURCE_MODE"] = "clone" - env["REPO_URL"] = r.serviceCloneURL(ctx, svc) + env["REPO_URL"] = repoURL env["REPO_BRANCH"] = env["BASE_BRANCH"] env["GIT_MODE"] = string(domain.GitModeReadonly) - if isAgent && svc.GitMode == domain.GitModeDraftPR && svc.RepoKind == domain.RepoKindProvider { + wantsPullRequest := false + if run.ExecutionContract != nil { + wantsPullRequest = run.ExecutionContract.Delivers(domain.OutputCreatePullRequest) + } else { + // Rolling-upgrade behavior for historical Runs without a contract. + wantsPullRequest = svc.GitMode == domain.GitModeDraftPR && svc.RepoKind == domain.RepoKindProvider + } + if isAgent && wantsPullRequest { env["GIT_MODE"] = string(domain.GitModeDraftPR) env["BRANCH_NAME"] = domain.RunPushBranch(run) } @@ -2452,5 +2524,7 @@ func (r *Reconciler) addGitEnv(ctx context.Context, env map[string]string, run * if run.Kind == domain.RunKindReview { env["PR_HEAD"] = run.PRHeadBranch env["PR_BASE"] = run.PRBaseBranch + env["PR_HEAD_SHA"] = run.PRHeadSHA + env["PR_BASE_SHA"] = run.PRBaseSHA } } diff --git a/orchestrator/internal/reconciler/session.go b/orchestrator/internal/reconciler/session.go index 2ad7d9d4..bc8bf20e 100644 --- a/orchestrator/internal/reconciler/session.go +++ b/orchestrator/internal/reconciler/session.go @@ -41,7 +41,7 @@ func (r *Reconciler) reconcileSessionPushes(ctx context.Context) { r.log.Warn("reconcile session push: get service", "run", run.ID, "err", err) continue } - if !sessionPushEligible(*svc) { + if !sessionPushEligible(run, *svc) { // readonly / raw session run: nothing to push. Advance pushed_rev so it // drops out of the scan (the diff artifact still uploaded per turn). if _, err := r.st.SetPushedRev(ctx, run.ID, run.BundleRev, run.CommitSHA); err != nil { @@ -56,7 +56,11 @@ func (r *Reconciler) reconcileSessionPushes(ctx context.Context) { // sessionPushEligible reports whether a session run's service pushes a draft PR // (draft_pr mode on a provider repo with owner/name). readonly/raw sessions are // diff-only. -func sessionPushEligible(svc domain.Service) bool { +func sessionPushEligible(run domain.Run, svc domain.Service) bool { + if run.ExecutionContract != nil { + return run.ExecutionContract.Delivers(domain.OutputCreatePullRequest) + } + // Rolling-upgrade behavior for historical Runs without a contract. return svc.GitMode == domain.GitModeDraftPR && svc.RepoKind == domain.RepoKindProvider && domain.ValidProvider(svc.Provider) && @@ -72,21 +76,22 @@ func (r *Reconciler) pushSessionRun(ctx context.Context, run *domain.Run, svc *d return // parked (P1): credential problem already surfaced once } rev := run.BundleRev // capture: a newer bundle after this leaves pushed_rev behind → re-push next tick - owner, repo, ok := provider.SplitRepo(svc.RepoOwnerName) - if !ok { - r.log.Warn("reconcile session push: bad repo_owner_name", "run", run.ID, "repo", svc.RepoOwnerName) - return - } - branch := run.GitBranch - tok, err := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) + scm, err := r.scmContextForRun(ctx, run, svc) if err != nil { - if errors.Is(err, credentials.ErrIntegrationCredential) { + if errors.Is(err, credentials.ErrIntegrationCredential) || errors.Is(err, credentials.ErrPluginCredentialUnavailable) { r.noteIntegrationCredentialFailure(ctx, run.ID, "session push", err) - return + } else { + r.log.Warn("reconcile session push: no frozen scm context", "run", run.ID, "err", err) } - r.log.Warn("reconcile session push: no credential; leaving for retry", "run", run.ID, "provider", svc.Provider, "err", err) return } + owner, repo, ok := provider.SplitRepo(scm.RepositoryPath) + if !ok { + r.log.Warn("reconcile session push: bad repository path", "run", run.ID, "repo", scm.RepositoryPath) + return + } + branch := run.GitBranch + tok := scm.Token bundle, err := r.st.GetRunBundle(ctx, run.ID) if err != nil { @@ -106,21 +111,16 @@ func (r *Reconciler) pushSessionRun(ctx context.Context, run *domain.Run, svc *d } f.Close() - rawURL := r.serviceCloneURL(ctx, svc) - if rawURL == "" { + if scm.CloneURL == "" { r.log.Warn("reconcile session push: could not derive push URL", "run", run.ID) return } - authed := tok.AuthedURL(rawURL, svc.Provider) + authed := tok.AuthedURL(scm.CloneURL, scm.Provider) if run.PRURL == "" { // First turn (or PR not opened yet): open the draft PR. An already-open PR // for this head wins (crash between push/create and persist). - prov, err := r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) - if err != nil { - r.log.Warn("reconcile session push: build client", "run", run.ID, "err", err) - return - } + prov := scm.Client pr, err := prov.FindOpenPRByHead(ctx, owner, repo, branch) if err != nil { if errors.Is(err, provider.ErrMultipleOpenPRs) { @@ -169,7 +169,7 @@ func (r *Reconciler) pushSessionRun(ctx context.Context, run *domain.Run, svc *d } if pr == nil { pr, err = prov.CreatePR(ctx, provider.CreatePRInput{ - Owner: owner, Repo: repo, Head: branch, Base: svc.DefaultBranch, + Owner: owner, Repo: repo, Head: branch, Base: scm.DefaultBranch, Title: prTitle(run.Prompt), Body: prBody(run, r.prTriggerAttribution(ctx, run, svc)), Draft: true, }) if err != nil { @@ -201,11 +201,7 @@ func (r *Reconciler) pushSessionRun(ctx context.Context, run *domain.Run, svc *d // Later turns: ff-only update the existing PR head branch (never force-push, // never open a new PR). - prov, err := r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) - if err != nil { - r.log.Warn("reconcile session push: build state client", "run", run.ID, "err", err) - return - } + prov := scm.Client current, err := prov.PRByNumber(ctx, owner, repo, run.PRNumber) if err != nil { r.log.Warn("reconcile session push: read provider state", "run", run.ID, "pr", run.PRNumber, "err", err) @@ -256,28 +252,24 @@ func (r *Reconciler) reconcileSessionPRReady(ctx context.Context) { r.log.Warn("reconcile session ready: get service", "run", run.ID, "err", err) continue } - if run.PRReadyPolicy != domain.PRReadyPolicyLifecycleAware || !sessionPushEligible(*svc) { + if effectivePRReadyPolicy(&run) != domain.PRReadyPolicyLifecycleAware || !sessionPushEligible(run, *svc) { continue } - owner, repo, ok := provider.SplitRepo(svc.RepoOwnerName) - if !ok { - r.log.Warn("reconcile session ready: bad repo_owner_name", "run", run.ID, "repo", svc.RepoOwnerName) - continue - } - tok, err := r.creds.ResolveForService(ctx, svc, run.TriggeredByUserID) + scm, err := r.scmContextForRun(ctx, &run, svc) if err != nil { - if errors.Is(err, credentials.ErrIntegrationCredential) { + if errors.Is(err, credentials.ErrIntegrationCredential) || errors.Is(err, credentials.ErrPluginCredentialUnavailable) { r.noteIntegrationCredentialFailure(ctx, run.ID, "mark PR ready", err) } else { - r.log.Warn("reconcile session ready: no credential", "run", run.ID, "err", err) + r.log.Warn("reconcile session ready: no frozen scm context", "run", run.ID, "err", err) } continue } - prov, err := r.factory.PRClient(svc.Provider, tok.Value, tok.Scheme) - if err != nil { - r.log.Warn("reconcile session ready: build client", "run", run.ID, "err", err) + owner, repo, ok := provider.SplitRepo(scm.RepositoryPath) + if !ok { + r.log.Warn("reconcile session ready: bad repository path", "run", run.ID, "repo", scm.RepositoryPath) continue } + prov := scm.Client pr, err := prov.MarkPRReady(ctx, owner, repo, run.PRNumber) if err != nil { if errors.Is(err, provider.ErrUnsupportedPRTransition) { diff --git a/orchestrator/internal/reconciler/session_test.go b/orchestrator/internal/reconciler/session_test.go index 98f9b6c6..f01c3db5 100644 --- a/orchestrator/internal/reconciler/session_test.go +++ b/orchestrator/internal/reconciler/session_test.go @@ -74,6 +74,7 @@ func TestSessionJobEnvAndTTL(t *testing.T) { ctx := context.Background() rec, st, fake := testRec(t, 10) rec.cfg.SessionTTLSecs = 7200 + store.ConfigureWorkflowTimeoutDefaults(st, rec.cfg.RunTimeoutSecs, rec.cfg.SessionTTLSecs) pid, sid := seedSessionProject(t, st, &domain.Project{}) run := &domain.Run{ID: domain.NewID(), ProjectID: pid, ServiceID: sid, Prompt: "chat", diff --git a/orchestrator/internal/scmevent/event.go b/orchestrator/internal/scmevent/event.go index 57aa9d48..67c59c40 100644 --- a/orchestrator/internal/scmevent/event.go +++ b/orchestrator/internal/scmevent/event.go @@ -96,6 +96,7 @@ type NormalizedSCMEvent struct { Ref string `json:"ref,omitempty"` BaseRef string `json:"base_ref,omitempty"` HeadSHA string `json:"head_sha,omitempty"` + BaseSHA string `json:"base_sha,omitempty"` Conclusion string `json:"conclusion,omitempty"` Body string `json:"body,omitempty"` OccurredAt time.Time `json:"occurred_at"` diff --git a/orchestrator/internal/scmevent/event_test.go b/orchestrator/internal/scmevent/event_test.go index 6c0bfa7f..51fdb189 100644 --- a/orchestrator/internal/scmevent/event_test.go +++ b/orchestrator/internal/scmevent/event_test.go @@ -153,7 +153,7 @@ func TestNormalizeGitHubCommentRequiresPullRequestMarker(t *testing.T) { func TestNormalizePullRequestCarriesDraftState(t *testing.T) { body := []byte(`{ "action":"opened", - "pull_request":{"id":8,"number":4,"draft":true,"head":{"ref":"feat","sha":"abc"},"base":{"ref":"main"}}, + "pull_request":{"id":8,"number":4,"draft":true,"head":{"ref":"feat","sha":"abc1234"},"base":{"ref":"main","sha":"def5678"}}, "repository":{"id":1,"full_name":"a/r"}, "sender":{"id":2,"login":"maintainer"} }`) @@ -164,6 +164,9 @@ func TestNormalizePullRequestCarriesDraftState(t *testing.T) { if !event.Draft { t.Fatal("draft pull request lost its draft state") } + if event.HeadSHA != "abc1234" || event.BaseSHA != "def5678" { + t.Fatalf("revision pair = %s...%s", event.BaseSHA, event.HeadSHA) + } } func TestNormalizeProviderLifecycleEvents(t *testing.T) { diff --git a/orchestrator/internal/scmevent/normalize.go b/orchestrator/internal/scmevent/normalize.go index ef78c87a..dce19e1f 100644 --- a/orchestrator/internal/scmevent/normalize.go +++ b/orchestrator/internal/scmevent/normalize.go @@ -95,6 +95,7 @@ type commonPayload struct { } `json:"head"` Base struct { Ref string `json:"ref"` + SHA string `json:"sha"` } `json:"base"` } `json:"pull_request"` Review struct { @@ -177,7 +178,7 @@ func NormalizeForApp(provider ProviderKind, eventType, deliveryID string, payloa event.Family = FamilyPullRequest event.Action = normalizePullRequestAction(p.Action, p.PullRequest.Merged, kind == "pull_request_sync") event.Object = Object{ID: anyID(p.PullRequest.ID), Number: p.PullRequest.Number, Title: p.PullRequest.Title, URL: p.PullRequest.HTMLURL} - event.Ref, event.BaseRef, event.HeadSHA = p.PullRequest.Head.Ref, p.PullRequest.Base.Ref, p.PullRequest.Head.SHA + event.Ref, event.BaseRef, event.HeadSHA, event.BaseSHA = p.PullRequest.Head.Ref, p.PullRequest.Base.Ref, p.PullRequest.Head.SHA, p.PullRequest.Base.SHA event.Draft = p.PullRequest.Draft case "pull_request_review": event.Family = FamilyReview @@ -289,6 +290,10 @@ type gitLabPayload struct { LastCommit struct { ID string `json:"id"` } `json:"last_commit"` + DiffRefs struct { + BaseSHA string `json:"base_sha"` + HeadSHA string `json:"head_sha"` + } `json:"diff_refs"` Status string `json:"status"` Ref string `json:"ref"` Tag string `json:"tag"` @@ -334,7 +339,9 @@ func normalizeGitLab(eventType, deliveryID string, payload []byte, receivedAt ti } event.Action = normalizeGitLabMRAction(p.ObjectAttributes.Action, p.ObjectAttributes.State) event.Object = Object{ID: anyID(p.ObjectAttributes.ID), Number: p.ObjectAttributes.IID, Title: p.ObjectAttributes.Title, URL: p.ObjectAttributes.URL} - event.Ref, event.BaseRef, event.HeadSHA = p.ObjectAttributes.SourceBranch, p.ObjectAttributes.TargetBranch, p.ObjectAttributes.LastCommit.ID + event.Ref, event.BaseRef = p.ObjectAttributes.SourceBranch, p.ObjectAttributes.TargetBranch + event.HeadSHA = firstNonEmpty(p.ObjectAttributes.DiffRefs.HeadSHA, p.ObjectAttributes.LastCommit.ID) + event.BaseSHA = p.ObjectAttributes.DiffRefs.BaseSHA if event.Action == ActionApproved || event.Action == ActionApprovalRemoved { event.Family = FamilyReview } diff --git a/orchestrator/internal/store/automation_execution.go b/orchestrator/internal/store/automation_execution.go index 39513e2f..f72315e7 100644 --- a/orchestrator/internal/store/automation_execution.go +++ b/orchestrator/internal/store/automation_execution.go @@ -353,6 +353,9 @@ func (m *MemStore) CreateAutomationExecution(_ context.Context, value *domain.Au if err := m.validateRunForCreateLocked(run); err != nil { return nil, false, err } + if err := m.resolveRunContractLocked(run); err != nil { + return nil, false, err + } } copyValue := *value m.automationExecutions[value.ID] = copyValue @@ -402,6 +405,9 @@ func (m *MemStore) ClaimPluginCronExecution( if err := m.validateRunForCreateLocked(run); err != nil { return false, err } + if err := m.resolveRunContractLocked(run); err != nil { + return false, err + } } cron.LastFiredAt = firedAt cron.LastError = execution.ReasonMessage diff --git a/orchestrator/internal/store/memory.go b/orchestrator/internal/store/memory.go index 727f09fc..1f1bcec1 100644 --- a/orchestrator/internal/store/memory.go +++ b/orchestrator/internal/store/memory.go @@ -18,6 +18,8 @@ import ( // a database. It is safe for concurrent use. type MemStore struct { mu sync.Mutex + workflowRunTimeoutSecs int64 + workflowSessionTTLSecs int64 projects map[string]domain.Project services map[string]domain.Service runs map[string]domain.Run @@ -82,6 +84,8 @@ type MemStore struct { // NewMemStore returns an empty in-memory store. func NewMemStore() *MemStore { return &MemStore{ + workflowRunTimeoutSecs: defaultWorkflowTimeoutSeconds, + workflowSessionTTLSecs: defaultWorkflowTimeoutSeconds, projects: map[string]domain.Project{}, services: map[string]domain.Service{}, runs: map[string]domain.Run{}, @@ -142,6 +146,17 @@ func NewMemStore() *MemStore { } } +func (m *MemStore) configureWorkflowTimeoutDefaults(runTimeoutSeconds, sessionTTLSeconds int64) { + m.mu.Lock() + defer m.mu.Unlock() + if runTimeoutSeconds > 0 { + m.workflowRunTimeoutSecs = runTimeoutSeconds + } + if sessionTTLSeconds > 0 { + m.workflowSessionTTLSecs = sessionTTLSeconds + } +} + // dedupeKey builds the per-source idempotency key mirroring the DB unique index // on (run_id, source, client_seq). func dedupeKey(runID, source string, clientSeq int64) string { @@ -557,6 +572,9 @@ func (m *MemStore) CreateRun(_ context.Context, r *domain.Run) error { if err := m.validateRunForCreateLocked(r); err != nil { return err } + if err := m.resolveRunContractLocked(r); err != nil { + return err + } m.insertRunLocked(r) return nil } @@ -579,6 +597,9 @@ func (m *MemStore) CreateCoalescedRun(_ context.Context, coalesceKey string, r * if err := m.validateRunForCreateLocked(r); err != nil { return err } + if err := m.resolveRunContractLocked(r); err != nil { + return err + } now := time.Now().UTC() for id, existing := range m.runs { if existing.CoalesceKey != coalesceKey || existing.Status != domain.StatusQueued { @@ -608,6 +629,9 @@ func normalizeRunForCreate(r *domain.Run) { // validateRunForCreateLocked performs every fallible check before insertion. // The caller holds m.mu. func (m *MemStore) validateRunForCreateLocked(r *domain.Run) error { + if r.Kind == domain.RunKindReview && (!domain.ValidCommitSHA(strings.TrimSpace(r.PRHeadSHA)) || !domain.ValidCommitSHA(strings.TrimSpace(r.PRBaseSHA))) { + return errors.New("review revision pair is required before queueing") + } // Older focused store tests seed standalone runs directly; preserve that // convenience while still enforcing the deletion fence whenever the service // aggregate is present (production PG always has the FK). @@ -651,10 +675,40 @@ func (m *MemStore) validateRunForCreateLocked(r *domain.Run) error { return nil } +func (m *MemStore) resolveRunContractLocked(r *domain.Run) error { + if r.ExecutionContract != nil { + if err := r.ExecutionContract.Validate(); err != nil { + return fmt.Errorf("create run: invalid execution contract: %w", err) + } + r.ExecutionContract = cloneWorkflowContract(r.ExecutionContract) + return nil + } + svc := m.services[r.ServiceID] + if svc.ID == "" { + svc = domain.Service{ID: r.ServiceID, ProjectID: r.ProjectID, RepoKind: domain.RepoKindRaw, DefaultBranch: "main", GitMode: domain.GitModeReadonly} + } + timeout, source := m.workflowRunTimeoutSecs, domain.TimeoutSourceCluster + project := m.projects[r.ProjectID] + if r.Session { + timeout = m.workflowSessionTTLSecs + if project.SessionTTLSecs != nil && *project.SessionTTLSecs > 0 { + timeout, source = *project.SessionTTLSecs, domain.TimeoutSourceProject + } + } else if project.RunTimeoutSecs != nil && *project.RunTimeoutSecs > 0 { + timeout, source = *project.RunTimeoutSecs, domain.TimeoutSourceProject + } + contract, err := domain.ResolveWorkflowContract(r, &svc, timeout, source, r.CreatedAt) + if err != nil { + return fmt.Errorf("create run: resolve execution contract: %w", err) + } + r.ExecutionContract = contract + return nil +} + // insertRunLocked applies the already-validated mutation. The caller holds // m.mu and must have called validateRunForCreateLocked first. func (m *MemStore) insertRunLocked(r *domain.Run) { - m.runs[r.ID] = *r + m.runs[r.ID] = cloneRun(*r) for _, id := range r.AttachmentStageIDs { stage := m.attachmentStages[id] m.runAttachments[r.ID] = append(m.runAttachments[r.ID], domain.RunAttachment{RunID: r.ID, StageID: stage.ID, ObjectKey: stage.ObjectKey, DisplayName: stage.DisplayName, ContentType: stage.ContentType, SizeBytes: stage.SizeBytes}) @@ -891,10 +945,43 @@ func (m *MemStore) GetRun(_ context.Context, id string) (*domain.Run, error) { if !ok { return nil, ErrNotFound } - cp := r + cp := cloneRun(r) return &cp, nil } +func cloneWorkflowContract(contract *domain.WorkflowContract) *domain.WorkflowContract { + if contract == nil { + return nil + } + copy := *contract + copy.Requirements = append([]string(nil), contract.Requirements...) + copy.Delivery.Outputs = append([]domain.WorkflowOutput(nil), contract.Delivery.Outputs...) + copy.Verification.RequiredRecords = append([]string(nil), contract.Verification.RequiredRecords...) + return © +} + +func cloneReviewPlan(plan *domain.ReviewPlan) *domain.ReviewPlan { + if plan == nil { + return nil + } + copy := *plan + copy.Files = append([]domain.ReviewPlanFile(nil), plan.Files...) + copy.Anchors = append([]domain.ReviewAnchor(nil), plan.Anchors...) + return © +} + +func cloneRun(run domain.Run) domain.Run { + run.ExecutionContract = cloneWorkflowContract(run.ExecutionContract) + run.ReviewPlan = cloneReviewPlan(run.ReviewPlan) + if run.ReviewResult != nil { + result := *run.ReviewResult + result.Findings = append([]domain.ReviewFinding(nil), run.ReviewResult.Findings...) + result.Checks = append([]string(nil), run.ReviewResult.Checks...) + run.ReviewResult = &result + } + return run +} + func (m *MemStore) GetRunByTokenHash(_ context.Context, tokenHash string) (*domain.Run, error) { m.mu.Lock() defer m.mu.Unlock() @@ -1068,8 +1155,18 @@ func (m *MemStore) ClaimRunDispatch(_ context.Context, id, jobName, tokenHash, p snap.ProviderBaseURL, snap.ProviderClientID, snap.ProviderClientSecretEnc = "", "", nil snap.ProviderAppID, snap.ProviderAppPrivateKeyEnc = "", nil snap.GitHubInstallID, snap.AccessTokenEnc, snap.RefreshTokenEnc, snap.TokenExpiresAt = "", nil, nil, nil + if binding, ok := m.serviceRepoBindings[run.ServiceID]; ok && binding.InstallationID == snap.InstallationID { + snap.RepositoryID, snap.RepositoryPath, snap.CloneURL, snap.DefaultBranch = binding.ProviderRepoID, binding.RepositoryPath, binding.CloneURL, binding.DefaultBranch + snap.ActingPrincipalKind, snap.ActingPrincipalID = "provider_bot", installation.ID + } staged[snap.InstallationID] = snap } + if requiredInstallationID != "" { + required, ok := staged[requiredInstallationID] + if !ok || !required.HasFrozenRepositoryGrant() { + return nil, fmt.Errorf("%w: required repository grant changed", ErrDispatchClaimUnavailable) + } + } m.runPluginSnapshots[id] = staged run.Status = domain.StatusScheduling run.Phase = phase @@ -1787,6 +1884,36 @@ func (m *MemStore) SetReviewResult(_ context.Context, id string, result domain.R return &out, nil } +func (m *MemStore) SetReviewPlan(_ context.Context, id string, plan domain.ReviewPlan) (*domain.Run, bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + cur, ok := m.runs[id] + if !ok { + return nil, false, ErrNotFound + } + if cur.Kind != domain.RunKindReview || (cur.Status != domain.StatusScheduling && cur.Status != domain.StatusRunning) { + return nil, false, ErrInvalidTransition + } + if cur.PRBaseSHA != "" && plan.BaseSHA != cur.PRBaseSHA || cur.PRHeadSHA != "" && plan.HeadSHA != cur.PRHeadSHA { + return nil, false, ErrConflict + } + want, err := plan.CanonicalHash() + if err != nil || plan.PlanHash != want { + return nil, false, ErrConflict + } + if cur.ReviewPlan != nil { + if cur.ReviewPlan.PlanHash != plan.PlanHash { + return nil, false, ErrConflict + } + out := cloneRun(cur) + return &out, false, nil + } + cur.ReviewPlan = cloneReviewPlan(&plan) + m.runs[id] = cur + out := cloneRun(cur) + return &out, true, nil +} + // MarkReviewPosted stamps review_posted_at idempotently, returning true only for // the caller that actually stamped it. func (m *MemStore) MarkReviewPosted(_ context.Context, id string) (bool, error) { diff --git a/orchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sql b/orchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sql new file mode 100644 index 00000000..f5993389 --- /dev/null +++ b/orchestrator/internal/store/migrations/0064_workflow_contract_review_plan.sql @@ -0,0 +1,17 @@ +-- 0064: immutable workflow execution contract and deterministic review input. +-- Existing rows remain NULL and are rendered as explicit legacy runs. +ALTER TABLE runs + ADD COLUMN IF NOT EXISTS execution_contract JSONB, + ADD COLUMN IF NOT EXISTS review_plan JSONB, + ADD COLUMN IF NOT EXISTS pr_head_sha TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS pr_base_sha TEXT NOT NULL DEFAULT ''; + +-- Repository and acting-principal facts are frozen with the same dispatch +-- snapshot as provider configuration and credential versions. +ALTER TABLE run_plugin_snapshots + ADD COLUMN IF NOT EXISTS repository_id TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS repository_path TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS clone_url TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS default_branch TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS acting_principal_kind TEXT NOT NULL DEFAULT '', + ADD COLUMN IF NOT EXISTS acting_principal_id TEXT NOT NULL DEFAULT ''; diff --git a/orchestrator/internal/store/pg.go b/orchestrator/internal/store/pg.go index a5c297b6..c39293c7 100644 --- a/orchestrator/internal/store/pg.go +++ b/orchestrator/internal/store/pg.go @@ -18,7 +18,9 @@ import ( // PGStore is a Postgres-backed Store using a pgx connection pool. type PGStore struct { - pool *pgxpool.Pool + pool *pgxpool.Pool + workflowRunTimeoutSecs int64 + workflowSessionTTLSecs int64 } // New opens a pool against dsn and returns a PGStore. Callers should run @@ -32,7 +34,16 @@ func New(ctx context.Context, dsn string) (*PGStore, error) { pool.Close() return nil, fmt.Errorf("ping db: %w", err) } - return &PGStore{pool: pool}, nil + return &PGStore{pool: pool, workflowRunTimeoutSecs: defaultWorkflowTimeoutSeconds, workflowSessionTTLSecs: defaultWorkflowTimeoutSeconds}, nil +} + +func (s *PGStore) configureWorkflowTimeoutDefaults(runTimeoutSeconds, sessionTTLSeconds int64) { + if runTimeoutSeconds > 0 { + s.workflowRunTimeoutSecs = runTimeoutSeconds + } + if sessionTTLSeconds > 0 { + s.workflowSessionTTLSecs = sessionTTLSeconds + } } // Pool exposes the underlying pool (used by Migrate at boot). @@ -439,12 +450,13 @@ const runCols = `id, project_id, service_id, prompt, status, kind, phase, error, result, model_id, model_name, base_branch, model_effort, goal_mode, session, awaiting_since, session_finalizing, bundle_rev, pushed_rev, permission_mode, - acp_session_id, resumed_from, provenance` + acp_session_id, resumed_from, provenance, + execution_contract, review_plan, pr_head_sha, pr_base_sha` func scanRun(row pgx.Row) (*domain.Run, error) { var r domain.Run var commentID, commentURL, automationID, eventKey, result *string - var reviewResult, provenanceSnapshot []byte + var reviewResult, provenanceSnapshot, executionContract, reviewPlan []byte err := row.Scan(&r.ID, &r.ProjectID, &r.ServiceID, &r.Prompt, &r.Status, &r.Kind, &r.Phase, &r.Error, &r.K8sJobName, &r.RetriedFrom, &r.FailureReason, &r.FailureMessage, &r.Attempt, &r.TokenHash, @@ -454,7 +466,8 @@ func scanRun(row pgx.Row) (*domain.Run, error) { &r.Origin, &commentID, &commentURL, &automationID, &eventKey, &r.CoalesceKey, &result, &r.ModelID, &r.ModelName, &r.BaseBranch, &r.ModelEffort, &r.GoalMode, &r.Session, &r.AwaitingSince, &r.SessionFinalizing, &r.BundleRev, &r.PushedRev, &r.PermissionMode, - &r.AcpSessionID, &r.ResumedFrom, &provenanceSnapshot) + &r.AcpSessionID, &r.ResumedFrom, &provenanceSnapshot, + &executionContract, &reviewPlan, &r.PRHeadSHA, &r.PRBaseSHA) if errors.Is(err, pgx.ErrNoRows) { return nil, ErrNotFound } @@ -489,6 +502,25 @@ func scanRun(row pgx.Row) (*domain.Run, error) { return nil, fmt.Errorf("scan run provenance: %w", err) } } + if len(executionContract) > 0 { + decoder := json.NewDecoder(strings.NewReader(string(executionContract))) + decoder.DisallowUnknownFields() + var contract domain.WorkflowContract + if err := decoder.Decode(&contract); err != nil { + return nil, fmt.Errorf("scan run execution contract: %w", err) + } + if err := contract.Validate(); err != nil { + return nil, fmt.Errorf("scan run execution contract: %w", err) + } + r.ExecutionContract = &contract + } + if len(reviewPlan) > 0 { + parsed, err := domain.UnmarshalStoredReviewPlan(reviewPlan) + if err != nil { + return nil, fmt.Errorf("scan run review plan: %w", err) + } + r.ReviewPlan = parsed + } return &r, nil } @@ -550,9 +582,18 @@ func (s *PGStore) CreateCoalescedRun(ctx context.Context, coalesceKey string, r } func (s *PGStore) createRunTx(ctx context.Context, tx pgx.Tx, r *domain.Run) error { + if r.Kind == domain.RunKindReview && (!domain.ValidCommitSHA(strings.TrimSpace(r.PRHeadSHA)) || !domain.ValidCommitSHA(strings.TrimSpace(r.PRBaseSHA))) { + return errors.New("create run: review revision pair is required before queueing") + } var deletingAt *time.Time var serviceReadyPolicy domain.PRReadyPolicy - if err := tx.QueryRow(ctx, `SELECT deleting_at,pr_ready_policy FROM services WHERE id=$1 FOR SHARE`, r.ServiceID).Scan(&deletingAt, &serviceReadyPolicy); errors.Is(err, pgx.ErrNoRows) { + var svc domain.Service + var providerName, repoOwnerName, rawRepoURL *string + var projectRunTimeout, projectSessionTTL *int64 + if err := tx.QueryRow(ctx, `SELECT s.deleting_at,s.pr_ready_policy,s.git_mode,s.repo_kind,s.provider,s.repo_owner_name,s.raw_repo_url,s.default_branch,p.run_timeout_secs,p.session_ttl_secs + FROM services s JOIN projects p ON p.id=s.project_id + WHERE s.id=$1 AND s.project_id=$2 FOR SHARE`, r.ServiceID, r.ProjectID).Scan( + &deletingAt, &serviceReadyPolicy, &svc.GitMode, &svc.RepoKind, &providerName, &repoOwnerName, &rawRepoURL, &svc.DefaultBranch, &projectRunTimeout, &projectSessionTTL); errors.Is(err, pgx.ErrNoRows) { return ErrNotFound } else if err != nil { return fmt.Errorf("create run: lock service: %w", err) @@ -566,13 +607,49 @@ func (s *PGStore) createRunTx(ctx context.Context, tx pgx.Tx, r *domain.Run) err r.PRReadyPolicy = domain.PRReadyPolicyAlwaysDraft } } + svc.ID, svc.ProjectID, svc.PRReadyPolicy = r.ServiceID, r.ProjectID, serviceReadyPolicy + if providerName != nil { + svc.Provider = domain.GitProvider(*providerName) + } + if repoOwnerName != nil { + svc.RepoOwnerName = *repoOwnerName + } + if rawRepoURL != nil { + svc.RawRepoURL = *rawRepoURL + } + if r.ExecutionContract != nil { + if err := r.ExecutionContract.Validate(); err != nil { + return fmt.Errorf("create run: invalid execution contract: %w", err) + } + } else { + timeout, source := s.workflowRunTimeoutSecs, domain.TimeoutSourceCluster + if timeout <= 0 { + timeout = defaultWorkflowTimeoutSeconds + } + if r.Session { + timeout = s.workflowSessionTTLSecs + if timeout <= 0 { + timeout = defaultWorkflowTimeoutSeconds + } + if projectSessionTTL != nil && *projectSessionTTL > 0 { + timeout, source = *projectSessionTTL, domain.TimeoutSourceProject + } + } else if projectRunTimeout != nil && *projectRunTimeout > 0 { + timeout, source = *projectRunTimeout, domain.TimeoutSourceProject + } + contract, contractErr := domain.ResolveWorkflowContract(r, &svc, timeout, source, r.CreatedAt) + if contractErr != nil { + return fmt.Errorf("create run: resolve execution contract: %w", contractErr) + } + r.ExecutionContract = contract + } provenanceJSON, err := json.Marshal(r.ProvenanceSnapshot) if err != nil { return fmt.Errorf("create run: encode provenance: %w", err) } _, err = tx.Exec(ctx, `INSERT INTO runs (`+runCols+`) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29,$30,$31,$32,$33,$34,$35,$36,$37,$38,$39,$40,$41,$42,$43,$44,$45,$46,$47,$48,$49,$50,$51,$52,$53,$54,$55)`, + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27,$28,$29,$30,$31,$32,$33,$34,$35,$36,$37,$38,$39,$40,$41,$42,$43,$44,$45,$46,$47,$48,$49,$50,$51,$52,$53,$54,$55,$56,$57,$58,$59)`, r.ID, r.ProjectID, r.ServiceID, r.Prompt, r.Status, string(r.Kind), r.Phase, r.Error, r.K8sJobName, r.RetriedFrom, r.FailureReason, r.FailureMessage, r.Attempt, r.TokenHash, r.CreatedAt, r.StartedAt, r.FinishedAt, r.JobCleanedAt, @@ -582,7 +659,8 @@ func (s *PGStore) createRunTx(ctx context.Context, tx pgx.Tx, r *domain.Run) err nullRunResult(r.Result), r.ModelID, r.ModelName, r.BaseBranch, r.ModelEffort, r.GoalMode, r.Session, r.AwaitingSince, r.SessionFinalizing, r.BundleRev, r.PushedRev, r.PermissionMode, - r.AcpSessionID, r.ResumedFrom, provenanceJSON) + r.AcpSessionID, r.ResumedFrom, provenanceJSON, + workflowContractJSON(r.ExecutionContract), reviewPlanJSON(r.ReviewPlan), r.PRHeadSHA, r.PRBaseSHA) if err != nil { return fmt.Errorf("create run: %w", err) } @@ -622,6 +700,25 @@ func reviewResultJSON(result *domain.ReviewResult) any { return encoded } +func workflowContractJSON(contract *domain.WorkflowContract) any { + if contract == nil { + return nil + } + encoded, err := json.Marshal(contract) + if err != nil { + return nil + } + return encoded +} + +func reviewPlanJSON(plan *domain.ReviewPlan) any { + encoded, err := domain.MarshalStoredReviewPlan(plan) + if err != nil || len(encoded) == 0 { + return nil + } + return encoded +} + func (s *PGStore) CreateAttachmentStage(ctx context.Context, stage *domain.AttachmentStage) error { tx, err := s.pool.Begin(ctx) if err != nil { @@ -1093,12 +1190,17 @@ func (s *PGStore) ClaimRunDispatch(ctx context.Context, id, jobName, tokenHash, createdAt = time.Now().UTC() } tag, insertErr := tx.Exec(ctx, ` - INSERT INTO run_plugin_snapshots(run_id,installation_id,provider,provider_config_revision,credential_version_id,created_at) - SELECT $1,pi.id,pi.provider,pi.config_revision,pi.credential_version_id,$3 + INSERT INTO run_plugin_snapshots(run_id,installation_id,provider,provider_config_revision,credential_version_id,created_at, + repository_id,repository_path,clone_url,default_branch,acting_principal_kind,acting_principal_id) + SELECT $1,pi.id,pi.provider,pi.config_revision,pi.credential_version_id,$3, + COALESCE(rb.provider_repo_id,''),COALESCE(rb.repository_path,''),COALESCE(rb.clone_url,''),COALESCE(rb.default_branch,''), + CASE WHEN rb.installation_id IS NULL THEN '' ELSE 'provider_bot' END, + CASE WHEN rb.installation_id IS NULL THEN '' ELSE pi.id END FROM plugin_installations pi JOIN provider_config_versions pv ON pv.provider=pi.provider AND pv.config_revision=pi.config_revision JOIN plugin_credential_versions cv ON cv.id=pi.credential_version_id AND cv.installation_id=pi.id AND cv.provider=pi.provider - WHERE pi.id=$2 AND pi.project_id=$4`, id, snap.InstallationID, createdAt, cur.ProjectID) + LEFT JOIN service_repository_bindings rb ON rb.service_id=$5 AND rb.installation_id=pi.id + WHERE pi.id=$2 AND pi.project_id=$4`, id, snap.InstallationID, createdAt, cur.ProjectID, cur.ServiceID) if insertErr != nil { err = insertErr return nil, fmt.Errorf("persist dispatch snapshot: %w", err) @@ -1107,6 +1209,20 @@ func (s *PGStore) ClaimRunDispatch(ctx context.Context, id, jobName, tokenHash, return nil, fmt.Errorf("%w: immutable version unavailable for plugin %s", ErrDispatchClaimUnavailable, snap.InstallationID) } } + if requiredInstallationID != "" { + var frozen bool + if err = tx.QueryRow(ctx, `SELECT EXISTS( + SELECT 1 FROM run_plugin_snapshots + WHERE run_id=$1 AND installation_id=$2 + AND repository_id<>'' AND repository_path<>'' AND clone_url<>'' AND default_branch<>'' + AND acting_principal_kind<>'' AND acting_principal_id<>'' + )`, id, requiredInstallationID).Scan(&frozen); err != nil { + return nil, fmt.Errorf("verify required repository grant: %w", err) + } + if !frozen { + return nil, fmt.Errorf("%w: required repository grant changed", ErrDispatchClaimUnavailable) + } + } if _, err = tx.Exec(ctx, `UPDATE runs SET status=$2,phase=$3,k8s_job_name=$4,token_hash=$5 WHERE id=$1`, id, domain.StatusScheduling, phase, jobName, tokenHash); err != nil { return nil, fmt.Errorf("claim run dispatch: %w", err) } @@ -1934,6 +2050,42 @@ func (s *PGStore) SetReviewResult(ctx context.Context, id string, result domain. return s.commitAndReload(ctx, tx, id) } +func (s *PGStore) SetReviewPlan(ctx context.Context, id string, plan domain.ReviewPlan) (*domain.Run, bool, error) { + tx, cur, err := s.lockRunTx(ctx, id) + if err != nil { + return nil, false, err + } + defer tx.Rollback(ctx) //nolint:errcheck + if cur.Kind != domain.RunKindReview || (cur.Status != domain.StatusScheduling && cur.Status != domain.StatusRunning) { + return nil, false, ErrInvalidTransition + } + if cur.PRBaseSHA != "" && plan.BaseSHA != cur.PRBaseSHA || cur.PRHeadSHA != "" && plan.HeadSHA != cur.PRHeadSHA { + return nil, false, ErrConflict + } + want, err := plan.CanonicalHash() + if err != nil || plan.PlanHash != want { + return nil, false, ErrConflict + } + if cur.ReviewPlan != nil { + if cur.ReviewPlan.PlanHash != plan.PlanHash { + return nil, false, ErrConflict + } + if err := tx.Commit(ctx); err != nil { + return nil, false, err + } + return cur, false, nil + } + encoded, err := domain.MarshalStoredReviewPlan(&plan) + if err != nil { + return nil, false, fmt.Errorf("marshal review plan: %w", err) + } + if _, err = tx.Exec(ctx, `UPDATE runs SET review_plan=$2::jsonb WHERE id=$1 AND review_plan IS NULL`, id, encoded); err != nil { + return nil, false, fmt.Errorf("set review plan: %w", err) + } + committed, err := s.commitAndReload(ctx, tx, id) + return committed, err == nil, err +} + // MarkReviewPosted stamps review_posted_at once the review comment has been // posted to the PR. Idempotent + first-writer-wins via COALESCE under a row // lock: returns posted=true only for the tick that actually stamped it, so two diff --git a/orchestrator/internal/store/pg_workflow_contract_integration_test.go b/orchestrator/internal/store/pg_workflow_contract_integration_test.go new file mode 100644 index 00000000..dd660293 --- /dev/null +++ b/orchestrator/internal/store/pg_workflow_contract_integration_test.go @@ -0,0 +1,77 @@ +package store + +import ( + "context" + "os" + "testing" + "time" + + "github.com/cnjack/jcloud/internal/domain" +) + +// Run with JCLOUD_TEST_DATABASE_URL to verify the JSONB contract/plan path +// against a real PostgreSQL instance. Ordinary unit suites skip it. +func TestPGWorkflowContractAndReviewPlanRoundTrip(t *testing.T) { + dsn := os.Getenv("JCLOUD_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("JCLOUD_TEST_DATABASE_URL is not set") + } + ctx := context.Background() + st, err := New(ctx, dsn) + if err != nil { + t.Fatal(err) + } + t.Cleanup(st.Close) + if err := Migrate(ctx, st.Pool()); err != nil { + t.Fatal(err) + } + ConfigureWorkflowTimeoutDefaults(st, 1800, 7200) + + project := &domain.Project{ID: domain.NewID(), Name: "workflow-contract-pg", CreatedAt: time.Now().UTC()} + if err := st.CreateProject(ctx, project); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = st.DeleteProject(ctx, project.ID) }) + service := &domain.Service{ + ID: domain.NewID(), ProjectID: project.ID, Name: "default", RepoKind: domain.RepoKindProvider, + Provider: domain.ProviderGitHub, RepoOwnerName: "acme/repo", DefaultBranch: "main", GitMode: domain.GitModeDraftPR, + CreatedAt: time.Now().UTC(), + } + if err := st.CreateService(ctx, service); err != nil { + t.Fatal(err) + } + run := &domain.Run{ + ID: domain.NewID(), ProjectID: project.ID, ServiceID: service.ID, Prompt: "review exact pair", + Status: domain.StatusQueued, Kind: domain.RunKindReview, PRNumber: 42, + PRHeadBranch: "feature", PRBaseBranch: "main", + PRHeadSHA: "2222222222222222222222222222222222222222", PRBaseSHA: "1111111111111111111111111111111111111111", + ModelName: "test/glm-5.2", Attempt: 1, CreatedAt: time.Now().UTC(), + } + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + if _, err := st.ScheduleRun(ctx, run.ID, "job-roundtrip", "token-hash", "PreparingWorkspace"); err != nil { + t.Fatal(err) + } + plan, err := domain.BuildReviewPlan(domain.ReviewPlanInput{ + BaseSHA: run.PRBaseSHA, HeadSHA: run.PRHeadSHA, MergeBaseSHA: run.PRBaseSHA, + Diff: "diff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1 +1,2 @@\n package main\n+var enabled = true\n", + CreatedAt: time.Now().UTC(), + }) + if err != nil { + t.Fatal(err) + } + if _, created, err := st.SetReviewPlan(ctx, run.ID, *plan); err != nil || !created { + t.Fatalf("set review plan: created=%v err=%v", created, err) + } + got, err := st.GetRun(ctx, run.ID) + if err != nil { + t.Fatal(err) + } + if got.ExecutionContract == nil || got.ExecutionContract.Workflow.ID != domain.BuiltinPullRequestReviewID { + t.Fatalf("execution contract did not round-trip: %+v", got.ExecutionContract) + } + if got.ReviewPlan == nil || got.ReviewPlan.PlanHash != plan.PlanHash || !got.ReviewPlan.AllowsAnchor("main.go", 2, 2) { + t.Fatalf("review plan did not round-trip with private anchors: %+v", got.ReviewPlan) + } +} diff --git a/orchestrator/internal/store/plugin_kanban_occurrence.go b/orchestrator/internal/store/plugin_kanban_occurrence.go index b3ec6423..4d674316 100644 --- a/orchestrator/internal/store/plugin_kanban_occurrence.go +++ b/orchestrator/internal/store/plugin_kanban_occurrence.go @@ -200,6 +200,9 @@ func (m *MemStore) CreatePluginKanbanOccurrenceRun(_ context.Context, occurrence if err := m.validateRunForCreateLocked(run); err != nil { return false, err } + if err := m.resolveRunContractLocked(run); err != nil { + return false, err + } m.insertRunLocked(run) now := time.Now().UTC() occurrence.RunID = run.ID diff --git a/orchestrator/internal/store/plugins.go b/orchestrator/internal/store/plugins.go index 683dd223..11f2c65c 100644 --- a/orchestrator/internal/store/plugins.go +++ b/orchestrator/internal/store/plugins.go @@ -1122,10 +1122,15 @@ func (s *PGStore) CreateRunPluginSnapshots(ctx context.Context, snapshots []doma // creates the immutable snapshot. This closes the gap between the // reconciler's list and insert when an owner disables/revokes a Plugin. if _, err = tx.Exec(ctx, ` - INSERT INTO run_plugin_snapshots(run_id,installation_id,provider,provider_config_revision,credential_version_id,created_at) - SELECT r.id,pi.id,pi.provider,pi.config_revision,pi.credential_version_id,$3 + INSERT INTO run_plugin_snapshots(run_id,installation_id,provider,provider_config_revision,credential_version_id,created_at, + repository_id,repository_path,clone_url,default_branch,acting_principal_kind,acting_principal_id) + SELECT r.id,pi.id,pi.provider,pi.config_revision,pi.credential_version_id,$3, + COALESCE(rb.provider_repo_id,''),COALESCE(rb.repository_path,''),COALESCE(rb.clone_url,''),COALESCE(rb.default_branch,''), + CASE WHEN rb.installation_id IS NULL THEN '' ELSE 'provider_bot' END, + CASE WHEN rb.installation_id IS NULL THEN '' ELSE pi.id END FROM runs r JOIN plugin_installations pi ON pi.project_id=r.project_id + LEFT JOIN service_repository_bindings rb ON rb.service_id=r.service_id AND rb.installation_id=pi.id JOIN provider_configs pc ON pc.provider=pi.provider AND pc.config_revision=pi.config_revision JOIN provider_config_versions pv ON pv.provider=pi.provider AND pv.config_revision=pi.config_revision JOIN plugin_credential_versions cv ON cv.id=pi.credential_version_id AND cv.installation_id=pi.id AND cv.provider=pi.provider @@ -1157,6 +1162,7 @@ func (s *PGStore) ClearQueuedRunPluginSnapshots(ctx context.Context, runID strin func (s *PGStore) ListRunPluginSnapshots(ctx context.Context, runID string) ([]domain.RunPluginSnapshot, error) { rows, err := s.pool.Query(ctx, `SELECT s.run_id,s.installation_id,s.provider,s.provider_config_revision,s.credential_version_id, + s.repository_id,s.repository_path,s.clone_url,s.default_branch,s.acting_principal_kind,s.acting_principal_id, COALESCE(pv.base_url,''),COALESCE(pv.client_id,''),pv.client_secret_enc, COALESCE(pv.app_id,''),pv.app_private_key_enc, COALESCE(cv.github_installation_id,''),cv.access_token_enc,cv.refresh_token_enc,cv.token_expires_at, @@ -1176,6 +1182,7 @@ func (s *PGStore) ListRunPluginSnapshots(ctx context.Context, runID string) ([]d var snap domain.RunPluginSnapshot if err := rows.Scan( &snap.RunID, &snap.InstallationID, &snap.Provider, &snap.ProviderConfigRevision, &snap.CredentialVersionID, + &snap.RepositoryID, &snap.RepositoryPath, &snap.CloneURL, &snap.DefaultBranch, &snap.ActingPrincipalKind, &snap.ActingPrincipalID, &snap.ProviderBaseURL, &snap.ProviderClientID, &snap.ProviderClientSecretEnc, &snap.ProviderAppID, &snap.ProviderAppPrivateKeyEnc, &snap.GitHubInstallID, &snap.AccessTokenEnc, &snap.RefreshTokenEnc, &snap.TokenExpiresAt, diff --git a/orchestrator/internal/store/plugins_test.go b/orchestrator/internal/store/plugins_test.go index e308abb1..e0ff8b16 100644 --- a/orchestrator/internal/store/plugins_test.go +++ b/orchestrator/internal/store/plugins_test.go @@ -635,7 +635,7 @@ func TestClaimRunDispatchIsAtomicAndFailureClearsCredentials(t *testing.T) { if err := st.CreateRun(ctx, run); err != nil { t.Fatal(err) } - if _, err := st.ClaimRunDispatch(ctx, run.ID, "job", "hash", "PreparingWorkspace", installation.ID, []domain.RunPluginSnapshot{{RunID: run.ID, InstallationID: installation.ID}}); err != nil { + if _, err := st.ClaimRunDispatch(ctx, run.ID, "job", "hash", "PreparingWorkspace", "", []domain.RunPluginSnapshot{{RunID: run.ID, InstallationID: installation.ID}}); err != nil { t.Fatal(err) } claimed, _ := st.GetRun(ctx, run.ID) @@ -657,6 +657,36 @@ func TestClaimRunDispatchIsAtomicAndFailureClearsCredentials(t *testing.T) { } } +func TestClaimRunDispatchRejectsChangedRequiredRepositoryGrant(t *testing.T) { + ctx, st := context.Background(), NewMemStore() + _ = st.CreateProject(ctx, &domain.Project{ID: "p", Name: "p"}) + _ = st.CreateService(ctx, &domain.Service{ID: "s", ProjectID: "p", Name: "s", RepoKind: domain.RepoKindProvider, Provider: domain.ProviderGitea, RepoOwnerName: "owner/repo", DefaultBranch: "main"}) + _ = st.UpsertProviderConfig(ctx, &domain.ProviderConfig{Provider: domain.PluginGitea, PluginEnabled: true}) + cfg, _ := st.GetProviderConfig(ctx, domain.PluginGitea) + installation := &domain.PluginInstallation{ID: "i", ProjectID: "p", Provider: domain.PluginGitea, Status: domain.PluginStatusEnabled, AccessTokenEnc: []byte("ciphertext"), ConfigRevision: cfg.ConfigRevision} + if err := st.CreatePluginInstallation(ctx, installation); err != nil { + t.Fatal(err) + } + if err := st.UpsertServiceRepositoryBinding(ctx, &domain.ServiceRepositoryBinding{ServiceID: "s", InstallationID: "i", ProviderRepoID: "42", RepositoryPath: "owner/repo", CloneURL: "https://git.example/owner/repo.git", DefaultBranch: "main"}); err != nil { + t.Fatal(err) + } + run := &domain.Run{ID: "r", ProjectID: "p", ServiceID: "s", Status: domain.StatusQueued} + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + if err := st.DeleteServiceRepositoryBinding(ctx, "s"); err != nil { + t.Fatal(err) + } + _, err := st.ClaimRunDispatch(ctx, "r", "job", "hash", "PreparingWorkspace", "i", []domain.RunPluginSnapshot{{RunID: "r", InstallationID: "i"}}) + if !errors.Is(err, ErrDispatchClaimUnavailable) { + t.Fatalf("claim err=%v want ErrDispatchClaimUnavailable", err) + } + got, _ := st.GetRun(ctx, "r") + if got.Status != domain.StatusQueued || got.TokenHash != "" { + t.Fatalf("failed grant claim mutated Run: %+v", got) + } +} + func TestCreateRunPluginSnapshotsIsAtomicInMemory(t *testing.T) { ctx, st := context.Background(), NewMemStore() _ = st.CreateProject(ctx, &domain.Project{ID: "p", Name: "p"}) @@ -841,7 +871,7 @@ func TestPGClaimRunDispatchFencesConcurrentDisableAndUninstall(t *testing.T) { } if err := st.CreatePluginBoundService(ctx, svc, &domain.ServiceRepositoryBinding{ ServiceID: svc.ID, InstallationID: installation.ID, ProviderRepoID: "claimed", - RepositoryPath: "owner/claimed", CreatedAt: time.Now(), + RepositoryPath: "owner/claimed", CloneURL: "https://gitea.example/owner/claimed.git", DefaultBranch: "main", CreatedAt: time.Now(), }); err != nil { t.Fatal(err) } diff --git a/orchestrator/internal/store/store.go b/orchestrator/internal/store/store.go index 2995e9e3..4e4ff2c1 100644 --- a/orchestrator/internal/store/store.go +++ b/orchestrator/internal/store/store.go @@ -37,6 +37,21 @@ var ErrAttachmentQuotaExceeded = errors.New("attachment staging quota exceeded") // the Plugin set it needs for launch (disabled, revoked, or reconfigured). var ErrDispatchClaimUnavailable = errors.New("dispatch claim unavailable") +const defaultWorkflowTimeoutSeconds int64 = 43200 + +// workflowTimeoutConfigurer is implemented by both stores. Production calls +// ConfigureWorkflowTimeoutDefaults once after loading cluster configuration; +// focused tests that omit it retain the documented 12-hour defaults. +type workflowTimeoutConfigurer interface { + configureWorkflowTimeoutDefaults(runTimeoutSeconds, sessionTTLSeconds int64) +} + +func ConfigureWorkflowTimeoutDefaults(st Store, runTimeoutSeconds, sessionTTLSeconds int64) { + if configurable, ok := st.(workflowTimeoutConfigurer); ok { + configurable.configureWorkflowTimeoutDefaults(runTimeoutSeconds, sessionTTLSeconds) + } +} + // EventInput is a single event to append. For AppendEvents the Seq is the // authoritative global seq (caller-assigned). For AppendRunnerEvents the Seq is // only the runner's client-side sequence number, used as a per-source @@ -378,6 +393,10 @@ type Store interface { SetReviewOutput(ctx context.Context, id, md string) (*domain.Run, error) // SetReviewResult records a validated structured result, first-writer-wins. SetReviewResult(ctx context.Context, id string, result domain.ReviewResult) (*domain.Run, error) + // SetReviewPlan persists the server-canonicalized deterministic review input. + // The first call creates it; an identical retry is a no-op; a different hash + // returns ErrConflict and can never replace the accepted plan. + SetReviewPlan(ctx context.Context, id string, plan domain.ReviewPlan) (*domain.Run, bool, error) // MarkReviewPosted stamps review_posted_at once the review comment is posted. // Idempotent + first-writer-wins: returns posted=true only for the tick that // stamped it, so two ticks never double-post. diff --git a/orchestrator/internal/store/workflow_contract_test.go b/orchestrator/internal/store/workflow_contract_test.go new file mode 100644 index 00000000..74582f24 --- /dev/null +++ b/orchestrator/internal/store/workflow_contract_test.go @@ -0,0 +1,92 @@ +package store + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/cnjack/jcloud/internal/domain" +) + +func TestMemStoreCreateRunResolvesContractWithEffectiveProjectTimeout(t *testing.T) { + ctx := context.Background() + st := NewMemStore() + ConfigureWorkflowTimeoutDefaults(st, 5400, 7200) + projectTimeout := int64(3600) + if err := st.CreateProject(ctx, &domain.Project{ID: "p", Name: "P", RunTimeoutSecs: &projectTimeout, CreatedAt: time.Now()}); err != nil { + t.Fatal(err) + } + if err := st.CreateService(ctx, &domain.Service{ID: "s", ProjectID: "p", Name: "default", RepoKind: domain.RepoKindProvider, Provider: domain.ProviderGitHub, RepoOwnerName: "cnjack/cloud", DefaultBranch: "main", GitMode: domain.GitModeDraftPR, PRReadyPolicy: domain.PRReadyPolicyLifecycleAware}); err != nil { + t.Fatal(err) + } + run := &domain.Run{ID: "r", ProjectID: "p", ServiceID: "s", Prompt: "implement", Status: domain.StatusQueued, Kind: domain.RunKindAgent, ModelName: "zhipu/glm-5.2", CreatedAt: time.Now()} + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + if run.ExecutionContract == nil { + t.Fatal("execution contract was not resolved") + } + if run.ExecutionContract.Execution.TimeoutSeconds != projectTimeout || run.ExecutionContract.Execution.TimeoutSource != domain.TimeoutSourceProject { + t.Fatalf("timeout = %#v", run.ExecutionContract.Execution) + } + stored, err := st.GetRun(ctx, run.ID) + if err != nil { + t.Fatal(err) + } + if stored.ExecutionContract == nil || stored.ExecutionContract.Hash != run.ExecutionContract.Hash { + t.Fatal("contract did not round-trip") + } +} + +func TestMemStoreRetryPreservesProvidedContract(t *testing.T) { + ctx := context.Background() + st := NewMemStore() + ConfigureWorkflowTimeoutDefaults(st, 5400, 7200) + if err := st.CreateProject(ctx, &domain.Project{ID: "p", Name: "P", CreatedAt: time.Now()}); err != nil { + t.Fatal(err) + } + svc := &domain.Service{ID: "s", ProjectID: "p", Name: "default", RepoKind: domain.RepoKindRaw, RawRepoURL: "https://example.invalid/repo.git", DefaultBranch: "main", GitMode: domain.GitModeReadonly} + if err := st.CreateService(ctx, svc); err != nil { + t.Fatal(err) + } + original := &domain.Run{ID: "r1", ProjectID: "p", ServiceID: "s", Prompt: "one", Status: domain.StatusQueued, Kind: domain.RunKindAgent, ModelName: "zhipu/glm-5.2", CreatedAt: time.Now()} + if err := st.CreateRun(ctx, original); err != nil { + t.Fatal(err) + } + retry := &domain.Run{ID: "r2", ProjectID: "p", ServiceID: "s", Prompt: "one", Status: domain.StatusQueued, Kind: domain.RunKindAgent, ModelName: "changed/model", RetriedFrom: &original.ID, ExecutionContract: original.ExecutionContract, CreatedAt: time.Now()} + if err := st.CreateRun(ctx, retry); err != nil { + t.Fatal(err) + } + if retry.ExecutionContract.Hash != original.ExecutionContract.Hash || retry.ExecutionContract.Execution.LLMSelection.ModelName != "zhipu/glm-5.2" { + t.Fatal("retry contract was re-resolved") + } +} + +func TestMemStoreSetReviewPlanFirstWriteIdempotency(t *testing.T) { + ctx := context.Background() + st := NewMemStore() + run := &domain.Run{ID: "r", ProjectID: "p", ServiceID: "s", Prompt: "review", Status: domain.StatusRunning, Kind: domain.RunKindReview, PRBaseSHA: strings.Repeat("a", 40), PRHeadSHA: strings.Repeat("b", 40)} + if err := st.CreateRun(ctx, run); err != nil { + t.Fatal(err) + } + plan, err := domain.BuildReviewPlan(domain.ReviewPlanInput{BaseSHA: run.PRBaseSHA, HeadSHA: run.PRHeadSHA, MergeBaseSHA: strings.Repeat("c", 40), Diff: "diff --git a/a.go b/a.go\n--- a/a.go\n+++ b/a.go\n@@ -1 +1 @@\n-old\n+new\n"}) + if err != nil { + t.Fatal(err) + } + _, created, err := st.SetReviewPlan(ctx, run.ID, *plan) + if err != nil || !created { + t.Fatalf("first write created=%v err=%v", created, err) + } + _, created, err = st.SetReviewPlan(ctx, run.ID, *plan) + if err != nil || created { + t.Fatalf("identical retry created=%v err=%v", created, err) + } + other := *plan + other.MergeBaseSHA = strings.Repeat("d", 40) + other.PlanHash, _ = other.CanonicalHash() + if _, _, err = st.SetReviewPlan(ctx, run.ID, other); !errors.Is(err, ErrConflict) { + t.Fatalf("conflict err = %v", err) + } +} diff --git a/outputs/workflow-platform-audit-2026-08-01.md b/outputs/workflow-platform-audit-2026-08-01.md new file mode 100644 index 00000000..7d947f27 --- /dev/null +++ b/outputs/workflow-platform-audit-2026-08-01.md @@ -0,0 +1,79 @@ +# Workflow Platform Ship-R1 audit + +日期：2026-08-01 +分支：`codex/workflow-platform` +范围：Workflow Contract、Run SCM Grant、确定性 PR Review、Console inspector、Runner protocol + +## 工具与实际模型 + +| 工具 | 请求模型 | 实际模型 | 说明 | +| --- | --- | --- | --- | +| GitHub Copilot CLI | Sonnet 5 | `auto` | 当前 CLI/账户不接受 `claude-sonnet-5`；没有把 auto 冒充 Sonnet 5 | +| Claude CLI | `glm-5.2` | `glm-5.2` | 精确模型，read-only plan permission | +| Grok CLI | `grok-4.5` | `grok-4.5` | 精确模型，禁止网络与编辑 | + +所有审计命令都被明确限制为 read-only；没有让外部 CLI 编辑、commit、push 或部署。 + +## 实现前审计 + +三类意见最终收敛到同一组 release invariant： + +1. 每个新 Run 必须在创建时冻结完整 execution contract； +2. 私有仓库的 clone、push、PR 和 review 必须共用 claim 时冻结的 SCM grant； +3. Review 必须固定 base/head SHA，模型前先生成 changed-line plan； +4. structured finding 必须命中 plan 的右侧 changed line； +5. Viewer projection 不暴露 Profile instructions、credential 或 private anchor； +6. Ready PR 是新流程默认语义，Draft 只作为 session/兼容策略。 + +## 实现后第一轮 + +- Copilot：没有发现可复现 P0；建议把 review result 的 legacy/structured 分支写得更明确，已重构。 +- Claude/GLM：`GO`，验证 migration/scan、所有创建路径、retry、anchor、idempotency、SCM consumer 和 UI projection；提出 model display、GitLab base label、marshal-error 三项 P2。 +- Grok：`NO-GO`，指出 4 项 P1：runtime 仍读 live GitMode、incomplete grant fallback、部分 Review 入口晚失败、contract Review 仍接受 text/plain。 + +处理结果：4 项全部修复并增加 regression。Plugin grant 的 claim 校验在 PG/Mem 中要求 repository id/path/clone/default branch/acting principal 全部存在，否则在 `queued → scheduling` 前原子失败。 + +## 实现后第二轮 + +- Claude/GLM：`GO`，4 项 blocker 全部清除，无 P0/P1。 +- Grok：确认 4 项 blocker 全部清除，但发现 GitLab MR webhook 常缺 + `diff_refs.base_sha`，自动 Review 没有补查 PR，判定为新 P1。 + +处理结果：统一 Plugin Review 在 revision 不完整时用 Service binding 的 provider +credential 调用 `PRByNumber`，拿到完整 pair 后才创建 Run；增加真实形状的 GitLab MR +webhook regression（输入不含 `diff_refs`，断言 provider lookup 恰好一次且 Run 冻结 +base/head SHA）。 + +## 最终复核 + +Grok `grok-4.5` 最终结论：`GO`。 + +复核证据： + +- GitLab no-`diff_refs` webhook 经 bound provider lookup 后可正常入队； +- one-shot、update-push、session push/ready 与 Runner `GIT_MODE` 均优先读取 immutable contract； +- Plugin-bound dispatch 在 claim transaction 内验证完整 frozen grant； +- Manual、legacy webhook、comment mention、unified Plugin webhook 和 Store safety net + 均在 queue 前拒绝缺失/非法 SHA； +- Contract Review 拒绝 text/plain，缺 plan 返回 `review_plan_required`，finding 经 + `ValidateAgainst` 校验； +- 没有剩余可复现 P0/P1。 + +额外采取的 P2 hardening：retry 复制 `PRReadyPolicy`；runtime Ready/Draft 决策优先读取 +contract 的 frozen ready policy；创建时校验 SHA 是 7–64 位十六进制；Viewer contract +projection 对 slice 做防御性 copy；Runner README 改为 `REVIEW.json`/Review Plan 协议。 + +## 验证记录 + +- Orchestrator：`go test ./...` 通过； +- PostgreSQL migration/round-trip：migration 0064、contract/hash、plan/anchor 与 snapshot + round-trip 通过（`JCLOUD_TEST_DATABASE_URL` gated integration）； +- Console：76 test files / 606 tests 通过；TypeScript typecheck、production build、design + token lint 通过； +- Runner Go modules：`acpdrive`、`orchclient`、`mockllm` 全部通过； +- Runner shell：syntax、delivery contract、session scrub matrix 通过； +- Runner container：credential-free source fetch、exact-SHA Review Plan、structured + `REVIEW.json` upload 通过；standalone no-TTY full loop 通过（最终复跑记录见交付）。 + +Console 测试仍打印既有 React `act(...)` 和 Router future-flag warning；Vite 仍提示大 +chunk。这些没有导致失败，也不是本次 Ship-R1 引入的 release blocker。 diff --git a/runner/README.md b/runner/README.md index ef066306..55b781a6 100644 --- a/runner/README.md +++ b/runner/README.md @@ -227,8 +227,14 @@ plane over the per-run `RUN_TOKEN`. (`BASE_BRANCH..BRANCH_NAME`), and POSTs it to `POST /internal/v1/runs/$RUN_ID/bundle` — the orchestrator pushes + opens the draft PR. **The runner never pushes and holds no token.** -- `PR_HEAD` / `PR_BASE` — review runs: the branches the runner diffs; the review - is written to `REVIEW.md` and POSTed to `POST /internal/v1/runs/$RUN_ID/review`. +- `PR_HEAD` / `PR_BASE` are display/checkout branch names for review runs; + `PR_HEAD_SHA` / `PR_BASE_SHA` are the required immutable revisions. The runner + verifies both commits, computes their merge-base, builds the unified diff, + and POSTs a server-validated Review Plan before starting the Agent. The Agent + writes structured `REVIEW.json`; the runner POSTs it as + `application/vnd.jcode.review+json` to + `POST /internal/v1/runs/$RUN_ID/review`. Contract review runs cannot fall back + to legacy Markdown output. When wired to an orchestrator it reads `ORCH_BASE_URL`, `RUN_TOKEN` (with `RUN_ID`) for all of the above plus event streaming and diff-artifact upload. diff --git a/runner/entrypoint.sh b/runner/entrypoint.sh index ff588f82..5cfaea7f 100755 --- a/runner/entrypoint.sh +++ b/runner/entrypoint.sh @@ -33,7 +33,8 @@ # BASE_BRANCH the baseline branch to check out (may be "") # GIT_MODE "readonly" (default; diff-only) | "draft_pr" # BRANCH_NAME the branch to create for a draft_pr bundle (jcode/run-) -# PR_HEAD/PR_BASE review run: the branches to diff (base...head) +# PR_HEAD/PR_BASE review run: display branches for the frozen revision pair +# PR_HEAD_SHA/PR_BASE_SHA exact review commits (required; branch movement is ignored) # # Orchestrator wiring (present under the control plane; absent standalone): # RUN_ID, RUN_TOKEN, ORCH_BASE_URL @@ -522,27 +523,42 @@ fi # The prompt contains the literal marker "[review]" so the mock LLM (and any # prompt-routing) can identify a review turn. if [ "$RUN_KIND" = "review" ]; then - [ -n "${PR_HEAD:-}" ] && [ -n "${PR_BASE:-}" ] \ - || die setup_failed "RUN_KIND=review requires PR_HEAD and PR_BASE" - log "review run: diffing $PR_BASE...$PR_HEAD" + [ -n "${PR_HEAD:-}" ] && [ -n "${PR_BASE:-}" ] && [ -n "${PR_HEAD_SHA:-}" ] && [ -n "${PR_BASE_SHA:-}" ] \ + || die setup_failed "review_revision_unavailable: RUN_KIND=review requires PR_HEAD, PR_BASE, PR_HEAD_SHA, and PR_BASE_SHA" + command -v git >/dev/null 2>&1 || die setup_failed "review_runtime_unavailable: git is required" + command -v rg >/dev/null 2>&1 || die setup_failed "review_runtime_unavailable: ripgrep is required" + if [ "${JCLOUD_PREP_ONLY:-0}" != "1" ]; then + command -v orchclient >/dev/null 2>&1 || die setup_failed "review_runtime_unavailable: review upload client is required" + fi + log "review run: verifying frozen revisions $PR_BASE_SHA...$PR_HEAD_SHA ($PR_BASE...$PR_HEAD)" # Make sure both refs are present (a bundle clone exposes them as origin/*). git -C "$WORKSPACE" fetch -q origin \ "+refs/heads/$PR_BASE:refs/remotes/origin/$PR_BASE" \ "+refs/heads/$PR_HEAD:refs/remotes/origin/$PR_HEAD" 2>/dev/null || true - HEAD_REF="origin/$PR_HEAD"; BASE_REF_R="origin/$PR_BASE" - git -C "$WORKSPACE" rev-parse --verify -q "$HEAD_REF" >/dev/null 2>&1 || HEAD_REF="$PR_HEAD" - git -C "$WORKSPACE" rev-parse --verify -q "$BASE_REF_R" >/dev/null 2>&1 || BASE_REF_R="$PR_BASE" + HEAD_REF="$(git -C "$WORKSPACE" rev-parse --verify -q "$PR_HEAD_SHA^{commit}" 2>/dev/null || true)" + BASE_REF_R="$(git -C "$WORKSPACE" rev-parse --verify -q "$PR_BASE_SHA^{commit}" 2>/dev/null || true)" + [ "$HEAD_REF" = "$PR_HEAD_SHA" ] || die setup_failed "review_revision_mismatch: frozen head commit is unavailable in the source bundle" + [ "$BASE_REF_R" = "$PR_BASE_SHA" ] || die setup_failed "review_revision_mismatch: frozen base commit is unavailable in the source bundle" + MERGE_BASE_SHA="$(git -C "$WORKSPACE" merge-base "$BASE_REF_R" "$HEAD_REF" 2>/dev/null || true)" + [ -n "$MERGE_BASE_SHA" ] || die setup_failed "review_revision_mismatch: no merge-base exists for the frozen revision pair" REVIEW_GIT_DIR="$(git -C "$WORKSPACE" rev-parse --absolute-git-dir 2>/dev/null)" \ || die setup_failed "review run could not resolve the workspace git directory" REVIEW_DIFF_FILE="$REVIEW_GIT_DIR/jcode-review.diff" - if ! git -C "$WORKSPACE" --no-pager diff "$BASE_REF_R...$HEAD_REF" > "$REVIEW_DIFF_FILE" 2>/dev/null; then + if ! git -C "$WORKSPACE" --no-pager diff "$MERGE_BASE_SHA" "$HEAD_REF" > "$REVIEW_DIFF_FILE" 2>/dev/null; then rm -f "$REVIEW_DIFF_FILE" 2>/dev/null || true - die setup_failed "review run could not compute diff $PR_BASE...$PR_HEAD" + die setup_failed "review run could not compute diff $MERGE_BASE_SHA...$PR_HEAD_SHA" fi REVIEW_DIFF_BYTES="$(wc -c < "$REVIEW_DIFF_FILE" | tr -d ' ')" + if [ "${JCLOUD_PREP_ONLY:-0}" != "1" ]; then + orchclient post-review-plan --diff "$REVIEW_DIFF_FILE" --base-sha "$PR_BASE_SHA" --head-sha "$PR_HEAD_SHA" --merge-base-sha "$MERGE_BASE_SHA" \ + || die setup_failed "review plan was rejected or could not be uploaded" + else + log "JCLOUD_PREP_ONLY=1: review plan upload skipped (no Agent turn will start)" + fi REVIEW_FOCUS="$TASK_PROMPT" TASK_PROMPT="$(cat < [flags]") + fmt.Fprintln(os.Stderr, "usage: orchclient [flags]") os.Exit(2) } cmd := os.Args[1] @@ -67,7 +70,7 @@ func main() { token := os.Getenv("RUN_TOKEN") if base == "" || runID == "" || token == "" { switch cmd { - case "fetch-source", "upload-bundle", "post-review": + case "fetch-source", "upload-bundle", "post-review-plan", "post-review": // Load-bearing: without the control plane these cannot do their job. fmt.Fprintln(os.Stderr, "[orchclient] "+cmd+" requires ORCH_BASE_URL/RUN_ID/RUN_TOKEN") os.Exit(1) @@ -182,6 +185,31 @@ func main() { os.Exit(1) } + case "post-review-plan": + fs := flag.NewFlagSet("post-review-plan", flag.ExitOnError) + diffFile := fs.String("diff", "", "path to the exact unified diff") + baseSHA := fs.String("base-sha", "", "frozen event base commit") + headSHA := fs.String("head-sha", "", "frozen event head commit") + mergeBaseSHA := fs.String("merge-base-sha", "", "computed merge-base commit") + _ = fs.Parse(args) + if *diffFile == "" || *baseSHA == "" || *headSHA == "" || *mergeBaseSHA == "" { + fmt.Fprintln(os.Stderr, "[orchclient] post-review-plan requires --diff, --base-sha, --head-sha, and --merge-base-sha") + os.Exit(2) + } + diff, err := os.ReadFile(*diffFile) + if err != nil { + fmt.Fprintf(os.Stderr, "[orchclient] post-review-plan: read %s: %v\n", *diffFile, err) + os.Exit(1) + } + body, err := json.Marshal(map[string]string{"base_sha": *baseSHA, "head_sha": *headSHA, "merge_base_sha": *mergeBaseSHA, "diff": string(diff)}) + if err != nil { + fmt.Fprintf(os.Stderr, "[orchclient] post-review-plan: encode: %v\n", err) + os.Exit(1) + } + if !c.uploadRaw("/internal/v1/runs/"+c.runID+"/review-plan", "application/json", body, "post-review-plan") { + os.Exit(1) + } + default: fmt.Fprintf(os.Stderr, "[orchclient] unknown command %q\n", cmd) os.Exit(2) diff --git a/runner/test-fetch-source.sh b/runner/test-fetch-source.sh index 90396421..6b83aac5 100755 --- a/runner/test-fetch-source.sh +++ b/runner/test-fetch-source.sh @@ -73,6 +73,8 @@ SEED="$TMP/seed"; OUT="$TMP/orch"; mkdir -p "$SEED" "$OUT" SRC_BUNDLE="$OUT/source.bundle" git -C "$SEED" bundle create "$SRC_BUNDLE" --all >/dev/null 2>&1 || fail "could not build source bundle" git bundle verify "$SRC_BUNDLE" >/dev/null 2>&1 || fail "source bundle invalid" +BASE_SHA="$(git -C "$SEED" rev-parse main)" +HEAD_SHA="$(git -C "$SEED" rev-parse feature)" cat > "$TMP/mock.py" <<'PY' import http.server, sys, os @@ -87,7 +89,10 @@ class H(http.server.BaseHTTPRequestHandler): self.send_response(404); self.end_headers() def do_POST(self): n = int(self.headers.get('Content-Length', '0')); body = self.rfile.read(n) - if self.path.endswith('/review'): + if self.path.endswith('/review-plan'): + open(os.path.join(OUT, 'received.review-plan'), 'wb').write(body) + self.send_response(201); self.end_headers(); self.wfile.write(b'{"coverage":"complete"}') + elif self.path.endswith('/review'): open(os.path.join(OUT, 'received.review'), 'wb').write(body) self.send_response(201); self.end_headers(); self.wfile.write(b'{"kind":"review"}') elif self.path.endswith('/bundle'): @@ -141,6 +146,7 @@ docker run --name "$RUN_CTR" --platform "linux/$TARGETARCH" \ -e SOURCE_MODE="fetch" \ -e BASE_BRANCH="main" \ -e PR_BASE="main" -e PR_HEAD="feature" \ + -e PR_BASE_SHA="$BASE_SHA" -e PR_HEAD_SHA="$HEAD_SHA" \ -e START_MOCKLLM=1 \ -e MODEL_NAME="mock/mock-model" -e MODEL_API_KEY="dummy-key" \ -e ORCH_BASE_URL="http://host.docker.internal:$MOCK_PORT" \ @@ -150,6 +156,7 @@ R_RC=$? set -e cat "$TMP/review.out" [ "$R_RC" -eq 0 ] || fail "[B] review run exited $R_RC (want 0)" +[ -s "$OUT/received.review-plan" ] || fail "[B] orchestrator did not receive a review plan before the review" [ -s "$OUT/received.review" ] || fail "[B] orchestrator did not receive a review" if ! grep -q '"findings"' "$OUT/received.review"; then echo "----- received review -----"; cat "$OUT/received.review" diff --git a/runner/test-integration.sh b/runner/test-integration.sh index a0bcddbc..d9dbb436 100755 --- a/runner/test-integration.sh +++ b/runner/test-integration.sh @@ -35,6 +35,7 @@ CONSOLE_TOKEN="itest-console-token" PG_DSN="${PG_DSN:-postgres://jcloud:jcloud@localhost:5432/jcloud?sslmode=disable}" SCENARIO="write_file" EXPECT_FILE="HELLO_FROM_JCODE.txt" +EXPECT_DIFF_PREFIX="JCODE_TASK_" EXPECT_STR="jcode ran headless in a container" pass() { printf '\033[32mPASS\033[0m %s\n' "$*"; } @@ -225,7 +226,7 @@ pass "event seqs are unique, gapless, monotonic (no collision / no drops)" # === 9. assert the artifact === ART_JSON="$(curl -fsS "${AUTH[@]}" "$API/runs/$RUN_ID/artifact")" || fail "artifact fetch failed" ART_CONTENT="$(printf '%s' "$ART_JSON" | JQ -r .content)" -printf '%s' "$ART_CONTENT" | grep -q "$EXPECT_FILE" || fail "artifact diff does not mention $EXPECT_FILE" +printf '%s' "$ART_CONTENT" | grep -q "$EXPECT_DIFF_PREFIX" || fail "artifact diff does not contain the per-task mock filename" printf '%s' "$ART_CONTENT" | grep -q "$EXPECT_STR" || fail "artifact diff missing scripted content" pass "artifact diff present and contains the scripted change" diff --git a/runner/test-session-scrub-matrix.sh b/runner/test-session-scrub-matrix.sh index fca60833..118613ad 100755 --- a/runner/test-session-scrub-matrix.sh +++ b/runner/test-session-scrub-matrix.sh @@ -70,7 +70,9 @@ run_case() { local extra_env=() if [ "$run_kind" = "review" ]; then - extra_env=(RUN_KIND=review PR_HEAD=main PR_BASE=main) + local revision + revision="$(git -C "$SEED" rev-parse main)" + extra_env=(RUN_KIND=review PR_HEAD=main PR_BASE=main PR_HEAD_SHA="$revision" PR_BASE_SHA="$revision") else extra_env=(RUN_KIND=agent) fi diff --git a/runner/test.sh b/runner/test.sh index b18ced2e..64009b29 100755 --- a/runner/test.sh +++ b/runner/test.sh @@ -23,6 +23,7 @@ MOCK_CTR="jcode-mockllm-test" RUN_CTR="jcode-runner-test" SCENARIO="write_file" EXPECT_FILE="HELLO_FROM_JCODE.txt" +EXPECT_DIFF_PREFIX="JCODE_TASK_" EXPECT_STR="jcode ran headless in a container" pass() { printf '\033[32mPASS\033[0m %s\n' "$*"; } @@ -117,10 +118,10 @@ pass "runner exited 0" [ -s "$OUT/diff.patch" ] || fail "/out/diff.patch is empty or missing" pass "diff.patch is non-empty ($(wc -c < "$OUT/diff.patch" | tr -d ' ') bytes)" -grep -q "$EXPECT_FILE" "$OUT/diff.patch" || fail "diff.patch does not mention $EXPECT_FILE" +grep -q "$EXPECT_DIFF_PREFIX" "$OUT/diff.patch" || fail "diff.patch does not contain the per-task mock filename" grep -q "$EXPECT_STR" "$OUT/diff.patch" || fail "diff.patch missing scripted content" grep -q '^new file mode' "$OUT/diff.patch" || fail "diff.patch is not a valid new-file diff" -pass "diff.patch contains the mock-scripted change to $EXPECT_FILE" +pass "diff.patch contains the per-task mock-scripted change" # stdout must carry the diff between markers too grep -q '===JCODE_DIFF_BEGIN' "$TMP/run.stdout" || fail "stdout missing diff markers"