diff --git a/.agents/agents/architect.md b/.agents/agents/architect.md index 45fe45686c..6bc38580dc 100644 --- a/.agents/agents/architect.md +++ b/.agents/agents/architect.md @@ -1,6 +1,6 @@ --- name: architect -description: Provide a frontier-model second opinion on unusually risky Kandev architecture and planning. The user-started primary planner owns normal specs, plans, and task decomposition. +description: User-requested independent frontier second opinion; the planner owns normal architecture, specs, plans, and task decomposition. tools: Bash, Read, Grep, Glob model: opus effort: high @@ -10,7 +10,7 @@ skills: context-engineering # Architect -Review one bounded architecture question from the primary planner. The planner +Invoke only when the user explicitly requests an independent architecture second opinion. Review one bounded architecture question from the primary planner. The planner owns design artifacts, decisions, user communication, and implementation orchestration. You do not edit files or implement production code. diff --git a/.agents/agents/code-review.md b/.agents/agents/code-review.md index bb9a2b80a0..29ecc092b8 100644 --- a/.agents/agents/code-review.md +++ b/.agents/agents/code-review.md @@ -1,6 +1,6 @@ --- name: code-review -description: Review changed code for quality, security, and architecture compliance. Use after implementing features or before opening PRs. +description: Frontier review for architecture, cross-cutting, high-risk, or stale/incomplete external-review Kandev changes. tools: Bash, Read, Grep, Glob model: opus effort: high diff --git a/.agents/agents/pr-poller.md b/.agents/agents/pr-poller.md index 634c812849..eeb9a25e95 100644 --- a/.agents/agents/pr-poller.md +++ b/.agents/agents/pr-poller.md @@ -17,6 +17,10 @@ Do not spawn subagents. ## Inputs The planner will tell you the PR number (or rely on `gh pr view` against the current branch). If neither is available, return a report with `error=...` and stop. +It may also provide `selected_reviewer=`. This is optional; without +it, use the normal configured-reviewer terminal condition. When the routine +OpenCode producer is selected as `${OPENCODE_REVIEW_APP_SLUG}[bot]`, require its exact-head +emitted `trusted_producer=true` record; a generic Actions review is not evidence. ## Output contract — print exactly this shape and nothing else @@ -32,6 +36,7 @@ ci_failed: - … (one entry per observed failure) ci_passed: ci_pending: +checks_evidence: checks_head_sha= snapshot_complete= bots: coderabbit: comments= greptile: reviews= @@ -39,8 +44,9 @@ bots: opencode: comments= cubic: reviews= unresolved_review_threads: -issue_comments_from_bots: +actionable_issue_comments_from_bots: claude_summary: blockers= suggestions= verdict= +review_evidence: reviewer= qualification= reviewed_head_sha= review_state= eligibility= verdict= trusted_producer= active_changes_requested= blocked_exact_current_head_reviews= recommendation: === end === ``` @@ -60,7 +66,7 @@ Do not hide it behind a generic `unknown` recommendation or retry it as a transient failure. Follow the access gate below and use its exact terminal recommendation. -The `claude_summary` line carries the **latest** Claude summary's structured findings table. Pure issue-comment counts (`issue_comments_from_bots`) miss this because the count alone can't tell the parent whether the comment is actionable (e.g. CodeRabbit's "review skipped, too many files" boilerplate ≠ a Claude finding). Use `claude_summary` to drive triage, not the raw count. +The `claude_summary` line carries the **latest** Claude summary's structured findings table. Pure issue-comment counts (`actionable_issue_comments_from_bots`) miss this because the count alone can't tell the parent whether the comment is actionable (e.g. CodeRabbit's "review skipped, too many files" boilerplate ≠ a Claude finding). Use `claude_summary` to drive triage, not the raw count. ## Procedure @@ -107,9 +113,9 @@ such as a DNS timeout or GitHub API failure, use the fetch-failure recovery path a. **Preferred path: raw snapshot.** ```bash - scripts/pr-state + scripts/pr-state --trusted-reviewer "$selected_reviewer" ``` - Parse `.checks`, `.review_threads`, `.unresolved_review_thread_count`, `.reviews`, `.issue_comments`, and `.errors`. Derive `ci_failed`, `ci_pending`, bot terminal states, and whether the latest bot summaries are actionable in the poller from those raw arrays. If `.errors` is non-empty, emit `unknown` for affected fields and explain the fetch failure in `recommendation:`. Do not backfill missing values from memory or from a generic CI template. + Parse `.checks`, `.checks_head_sha`, `.checks_snapshot_complete`, `.review_threads`, `.unresolved_review_thread_count`, `.reviews`, `.issue_comments`, `.review_evidence`, and `.errors`. Derive `ci_failed`, `ci_pending`, bot terminal states, and whether the latest bot summaries are actionable in the poller from those raw arrays. The PR-view check snapshot is current-head complete only when `checks_snapshot_complete=true`; its `checks_head_sha` must equal both review evidence opening/closing heads before selected-review evidence can qualify. `.review_evidence` is exact-head evidence only when all three heads are known and identical; `current_head_sha` is the final observed head, while `evidence_head_sha` is present only for a stable combined collection. If check snapshot H1 and reviews are stable H2, emit `qualification=unknown` and retry/fall back; never pair their CI and review state. Only records whose `commit_id` equals `evidence_head_sha` qualify; timestamps never prove head coverage. `blocked_exact_current_head_reviews` aggregates every exact-head record with `eligibility=blocked`, regardless of author; old-head blockers are excluded. Use the record's machine-readable `eligibility` and `verdict`: approved is eligible only absent a blocker signal, commented is eligible only with ``, nonzero structured blockers, independently labeled Markdown `Blocker:`/`Blockers:` lines with descriptive values, or explicit blocked/changes-requested/action-required/must-fix evidence are blocked. `No blocker(s): ...`, `Blocker(s): 0`, and `| Blocker | 0 |` are nonblocking; later independent positive label lines still win. Dismissed/pending/unknown or ambiguous comments are never eligible. If `.errors` is non-empty, emit `unknown` for affected fields and explain the fetch failure in `recommendation:`. Do not backfill missing values from memory or from a generic CI template. b. **Fallback raw CI status:** ```bash @@ -153,17 +159,18 @@ such as a DNS timeout or GitHub API failure, use the fetch-failure recovery path - `done` if a matching review exists OR if `statusCheckRollup` shows the `cubic · AI code reviewer` check completed - else `pending` - - **OpenCode** (`github-actions[bot]`, usually inline comments via a trusted wrapper; fallback issue comments are optional and may be absent): + - **OpenCode** (`${OPENCODE_REVIEW_APP_SLUG}[bot]`, only for routine same-repository PRs with the configured App and a successful full-PR check): ```bash gh pr view --json statusCheckRollup gh pr view --json comments --jq '.comments[] | select(.author.login == "github-actions" and (.body | startswith("## OpenCode review")))' ``` - - `done` primarily from `statusCheckRollup` when `opencode-review-same-repo`, `opencode-review-fork`, or the `OpenCode Code Review` workflow completed + - For the selected dedicated App, use only `review_evidence.trusted_workflow_run`; displayed check/workflow names do not authenticate evidence. - also `done` if a fallback issue comment starts with `## OpenCode review`, but do not require that comment - else `pending` d. **Exit conditions:** - - All CI checks completed AND every bot is in a terminal state (`done` / `rate_limited` / `timeout`) → exit loop. + - With `selected_reviewer`, use `scripts/pr-state --trusted-reviewer "$selected_reviewer" `. After all CI checks complete, exit when `checks_snapshot_complete=true`, all three evidence heads match, its exact-current-head record has `eligibility=eligible`, and active changes requests, actionable unresolved threads, actionable issue comments, and `blocked_exact_current_head_review_count` are all known zero (`qualification=qualified`). Only the dedicated OpenCode App `${OPENCODE_REVIEW_APP_SLUG}[bot]` additionally requires emitted `trusted_producer=true`; it means both the raw dedicated-App marker predicate and exact authenticated workflow/job success passed. For other explicitly selected reviewers, `trusted_producer` is not a qualification gate. Any missing/failed API state or a latest non-success workflow/job requires local frontier fallback only for the dedicated App path; ambiguous/ineligible selected evidence always requires fallback. + - Without `selected_reviewer`, all CI checks completed AND every configured reviewer is in a terminal state (`done` / `rate_limited` / `timeout`) → exit loop. - Round 40 reached (≈20 min) → mark any still-pending CI checks under `ci_pending:` and any still-pending bots as `timeout`, then exit loop. 4. **Fallback only: count unresolved review threads** via GraphQL (single call, not per-round). Skip this when `scripts/pr-state` already returned `.unresolved_review_thread_count` without a `review_threads` error: @@ -206,9 +213,9 @@ such as a DNS timeout or GitHub API failure, use the fetch-failure recovery path If `body` was empty (no Claude summary yet), emit `claude_summary: blockers=0 suggestions=0 verdict=none`. Default missing counts to `0`. -7. **Emit the report.** Fill in the shape above exactly. Use green wording only +7. **Emit the report.** Fill in the shape above exactly. Emit `checks_evidence` from `checks_head_sha`/`checks_snapshot_complete`; when it is not complete, do not describe CI as current-head green. For `review_evidence`, use `reviewer=none qualification=unqualified reviewed_head_sha= review_state=none eligibility=ineligible verdict=none trusted_producer=unknown active_changes_requested= blocked_exact_current_head_reviews=` when no reviewer was selected. With a selected reviewer, report its latest exact-current-head record's `review_state`, `eligibility`, and `verdict`, and set `trusted_producer` from the canonical raw field. Emit `qualified` only when the PR-view check head plus review opening/closing heads are known, identical, and complete, that record is `eligible`, and active changes-request, actionable-thread, and blocked-exact-review counts are known zero; only the dedicated OpenCode App additionally requires `trusted_producer=true`. For non-OpenCode selected reviewers, neither `trusted_producer=false` nor `unknown` changes generic qualification. Emit `blocked` when any such count is nonzero or the selected record is `blocked`, `unqualified` for an ineligible/ambiguous/no selected record (or a dedicated App without `trusted_producer=true`), and `unknown` when evidence/head data is incomplete. A changed head invalidates every prior record. Use green wording only when the report is complete and every required value is known: `ci_failed` - is known empty, `ci_pending` is `none`, every bot is `done` or + is known empty, `ci_pending` is `none`, every configured reviewer is done or `rate_limited`, mergeability and local conflict state are known clean, and all required counts are known. The `recommendation:` line is one short sentence chosen from this menu, picking the first that applies: @@ -218,6 +225,8 @@ such as a DNS timeout or GitHub API failure, use the fetch-failure recovery path - `"Claude summary flags blocker(s); planner should assign comment triage and remediation."` if `claude_summary.blockers > 0` - `"Polling timed out with pending items; planner should decide whether to launch another poll."` if any axis hit the cap - `"PR state fetch failed; planner should retry polling."` if an approved request ran but a required fetch failed or any required value is `unknown` + - `"Selected review blocks the PR; planner should assign comment triage and remediation."` if selected-review `qualification` is `blocked` + - `"Selected exact-head review is qualified; planner may continue without waiting for unrelated bots."` if selected-review `qualification` is `qualified` - `"Polling is incomplete; planner should continue polling."` if CI or any bot is still pending - `"All checks green; planner should assign triage for unresolved review threads."` if `unresolved_review_threads > 0` - `"All threads resolved; Claude has pending suggestion(s) — planner should assign triage."` if `claude_summary.suggestions > 0` diff --git a/.agents/agents/qa.md b/.agents/agents/qa.md index b55920b8d8..11af46ac48 100644 --- a/.agents/agents/qa.md +++ b/.agents/agents/qa.md @@ -1,6 +1,6 @@ --- name: qa -description: Verify integrated Kandev work after implementation. Use after spec-driven task waves or before PRs to check wiring, behavior, edge cases, test coverage, and readiness. +description: Independently verify integration, public-contract, persistence, concurrency, recovery, cross-component, or insufficiently evidenced Kandev changes. tools: Bash, Read, Grep, Glob model: sonnet effort: medium diff --git a/.agents/agents/simplify.md b/.agents/agents/simplify.md index 16c1cf06cd..184c6eee6a 100644 --- a/.agents/agents/simplify.md +++ b/.agents/agents/simplify.md @@ -63,4 +63,4 @@ Report what was simplified: - What was removed/inlined/simplified - Lines removed (net) -Do not spawn subagents. Report required full verification to the planner. +Do not spawn subagents. Report required change-aware verification to the planner. diff --git a/.agents/agents/verify.md b/.agents/agents/verify.md index 2e47ded303..37ca3637ff 100644 --- a/.agents/agents/verify.md +++ b/.agents/agents/verify.md @@ -1,6 +1,6 @@ --- name: verify -description: Run Kandev format, typecheck, tests, and lint before commit, then return a compact pass/fail report without fixing source failures. +description: Verify the committed Kandev artifact before push, reusing proven hook coverage and reporting a compact pass/fail result. tools: Bash, Read, Grep, Glob model: sonnet effort: low @@ -9,7 +9,11 @@ permissionMode: acceptEdits # Verify -Run the full verification pipeline for the monorepo and report any failures. +Run changed-scope verification by default and report mode, paths, commands, and +coverage limits. Read +`.agents/skills/verify/references/impact-matrix.md` and, when supplied, +`hook-evidence.md`; use full mode only for explicit broad/ambiguous triggers. +Scoped success is `changed-scope PASS`. Do not change production or test logic, resolve conflicts, rebase, or commit. `permissionMode: acceptEdits` is intentional so the Bash-driven `make fmt` @@ -28,17 +32,39 @@ step can retain formatter changes. It does not authorize source or test fixes. `origin/$PR_BASE` is an ancestor of `HEAD`; do not rebase or resolve conflicts in this role. -2. **Format and generate metadata:** +2. **Resolve verification scope and hook evidence:** + - Prefer the supplied last verified SHA as scope base only when it is an + ancestor of `HEAD`; otherwise use `origin/$PR_BASE`. + - Validate any supplied `/commit` receipt against current `HEAD`, `HEAD^`, + the scope base, hook results, bypass state, and a clean worktree exactly as + required by + `.agents/skills/verify/references/hook-evidence.md`. Never infer a receipt. + - Collect the scope delta and any dirty paths: + ```bash + git diff --name-only "$SCOPE_BASE"...HEAD + git diff --name-only + git diff --cached --name-only + git ls-files --others --exclude-standard + ``` + Categorize the union with + `.agents/skills/verify/references/impact-matrix.md`. Use + `mode=changed` unless that matrix requires `mode=full`. If the base or diff + cannot be resolved, fail closed to full mode. + Build the omitted-check list before executing commands. Once a receipt is + eligible, running a covered duplicate is a procedure failure; do not rerun + it for reassurance. + +3. **Run only the selected commands** through `scripts/run-quiet`. + - In changed mode, run uncovered matrix commands for impacted categories. + Report every omitted command and its exact covering hook. + Generate web metadata only when web/shared TypeScript is impacted. Run + only the applicable formatter and review any formatter changes. + - In full mode, ignore hook omissions and run: ```bash scripts/run-quiet format -- make fmt git status --short node apps/web/scripts/generate-release-notes.mjs node apps/web/scripts/generate-changelog.mjs - ``` - Review formatter changes before continuing. - -3. **Run the complete pipeline** through `scripts/run-quiet`: - ```bash scripts/run-quiet typecheck -- make typecheck scripts/run-quiet test -- make test scripts/run-quiet lint -- make lint @@ -56,7 +82,7 @@ step can retain formatter changes. It does not authorize source or test fixes. - If Go tests fail because `httptest.NewServer` cannot bind loopback in a restricted sandbox, rerun the exact command with normal network/loopback escalation before diagnosing code. - If required filesystem, network, or loopback escalation is unavailable, denied, cancelled, or interrupted, stop. Report verification as blocked, - explain that mandatory verification is preventing commit and PR creation, + explain that mandatory verification is preventing push and PR delivery, and include a **Required user action** telling the user to enable the runtime's full access mode and retry verification. Do not offer to proceed unverified or imply that the agent or repository host cannot create PRs. @@ -69,11 +95,14 @@ step can retain formatter changes. It does not authorize source or test fixes. remediation. 5. **Stop** after a reproducible source/test failure is captured. The planner - assigns remediation and then launches a fresh verification run. + handles a small remediation directly or delegates a larger one, then + launches a fresh verification run. -6. **Done** only when base ancestry has been reported and format, metadata - generation, typecheck, the complete test target, lint, and any scoped Rust - tests all pass. +6. **Done** only when PR/scope bases, ancestry, mode, receipt eligibility, + omissions, changed paths/categories, exact commands, and coverage limits are + reported and every selected check passes. If formatting changed the + post-commit checkout, invalidate the pass until a new commit and verification + run. Report `changed-scope PASS` or `full PASS` accurately. Do not spawn subagents. Report pass/fail state, blockers, and any required user action to the planner. diff --git a/.agents/skills/commit/SKILL.md b/.agents/skills/commit/SKILL.md index 5737090fb7..1eb9ef41eb 100644 --- a/.agents/skills/commit/SKILL.md +++ b/.agents/skills/commit/SKILL.md @@ -7,18 +7,17 @@ description: Stage and commit changes using Conventional Commits. Use when there ## Planner Entry -The user-started primary session delegates full -verification to `verify`, then delegates staging and committing the verified -checkout to an `implementer` worker. It does not run Git commands directly. The -commit worker must receive the successful verification result and must not spawn -other workers. +The planner may stage and commit routine changes directly after their focused +implementation checks pass. Final delegated `verify` runs on the committed +artifact before push. Return the hook receipt defined below so `verify` can +avoid repeating checks that successful hooks already covered. Create a git commit following this project's Conventional Commits convention. These messages are used by git-cliff (`cliff.toml`) to auto-generate changelogs and release notes. PRs are squash-merged, so the PR title becomes the commit on `main` — CI validates it via `pr-title.yml`. -## Available skills and subagents +## Available skills -- **`verify` worker** — The planner runs this prerequisite before assigning the - commit worker. +- **`verify` worker** — The planner runs this post-commit, pre-push gate with + the hook receipt and last verified SHA. ## Format @@ -73,8 +72,11 @@ explicitly requests task tracking. pre-commit --version >/dev/null 2>&1 && echo "INSTALLED" || echo "NOT_INSTALLED" # Is the hook actually wired into git's hook system? - HOOK_PATH=$(git rev-parse --git-path hooks/pre-commit) - test -f "$HOOK_PATH" && grep -q "pre-commit" "$HOOK_PATH" && echo "ACTIVE" || echo "INACTIVE" + PRE_COMMIT_HOOK_PATH=$(git rev-parse --git-path hooks/pre-commit) + COMMIT_MSG_HOOK_PATH=$(git rev-parse --git-path hooks/commit-msg) + test -f "$PRE_COMMIT_HOOK_PATH" && grep -q "pre-commit" "$PRE_COMMIT_HOOK_PATH" \ + && test -f "$COMMIT_MSG_HOOK_PATH" && grep -q "pre-commit" "$COMMIT_MSG_HOOK_PATH" \ + && echo "ACTIVE" || echo "INACTIVE" ``` - If **NOT_INSTALLED**, tell the user once: _"⚠️ pre-commit is not on PATH. Install it with `pip install pre-commit` so format/lint runs on every commit."_ Then continue (don't block). @@ -87,13 +89,33 @@ explicitly requests task tracking. Why this matters: a missing hook lets lint regressions slip past local commits and only surface in CI (e.g. funlen / cognitive complexity on backend Go code). The hook catches them in <1s at commit time. See `Makefile`'s `doctor` target for the idempotent install command. -3. **Require verify (MANDATORY — do NOT skip):** Confirm the planner supplied a - successful `verify` worker result for the current checkout. If the checkout - changed afterward or no result was supplied, stop and report that a new - verification assignment is required. Do not spawn or run verification from - the commit worker. +3. **Capture the parent SHA and preserve hook evidence:** + ```bash + git rev-parse HEAD + ``` + Never use `--no-verify`, `SKIP`, or another hook-bypass option or + environment variable. A bypassed commit is allowed only when the user + explicitly requests it, and its receipt must say `bypass: true`; it never + qualifies for hook-aware verification omissions. 4. **Stage files:** Stage relevant files (prefer specific files over `git add -A`). - **Splitting commits with new files:** When introducing a brand-new file alongside the file that uses it, stage them together. The Go lint pre-commit hook stashes *unstaged* changes before linting but keeps *untracked* files in the working tree — so a new helper committed alone, while its (still-unstaged) caller sits in the working tree, lints as `unused` and rejects the commit. 5. **Commit:** Write a commit message following the format above. If changes span multiple concerns, consider separate commits. + If a formatter changes files and prevents the commit, review and re-stage + those files, then create a new commit attempt; do not use `--amend`. + +6. **Return a hook receipt:** After a successful commit, report: + ```text + parent_sha: + commit_sha: + pre_commit_hook: active|inactive + commit_msg_hook: active|inactive + hook_results: + bypass: false|true + commit_result: pass + worktree: clean|dirty + ``` + `verify` may use the receipt only when both hooks are active, bypass is + false, the commit succeeded, the current `HEAD` still equals `commit_sha`, + and the worktree is clean. The commit worker does not run verification. diff --git a/.agents/skills/debug/SKILL.md b/.agents/skills/debug/SKILL.md index 5b52b54c52..69ab9878d3 100644 --- a/.agents/skills/debug/SKILL.md +++ b/.agents/skills/debug/SKILL.md @@ -10,10 +10,9 @@ Diagnose efficiently and safely. Debugging produces evidence and a root-cause hy ## Planner Entry -In the user-started primary session, delegate the -triage, evidence gathering, and diagnosis below to one `implementer` worker with -production edits forbidden. Review its evidence and decide whether to create a -separate `/fix` assignment. Do not run the diagnostic procedure directly. +The planner may diagnose a small, bounded issue directly. Delegate broad or +unknown exploration and long/noisy debugging to one `implementer` with +production edits forbidden; then decide whether `/fix` is needed. An explicitly assigned worker follows the remaining procedure, cleans up its temporary artifacts, reports evidence, and does not spawn other workers. diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index 3111c862ec..3e188231f2 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -7,10 +7,9 @@ description: Write and run web E2E tests (Playwright) using TDD — locations, p ## Execution Context -The user-started primary session delegates this -procedure to an `implementer` or `test-engineer` worker and does not write or run -E2E tests directly. An explicitly assigned worker continues below and does not -spawn other workers. +The planner may write or run a quick focused E2E check for small scoped work; +delegate long, noisy, broad, or parallel E2E work. A worker owns one packet and +does not spawn workers. Write E2E tests using TDD (Red-Green-Refactor). Always run the tests you create and watch them fail before implementing. @@ -396,5 +395,5 @@ Follow `/tdd` when writing E2E tests: 1. **RED** — Write the spec, run it, watch it fail (missing `data-testid`, feature not implemented, etc.) 2. **GREEN** — Implement the feature/fix, add `data-testid` attributes, run the test until green 3. **REFACTOR** — Extract page objects, clean up selectors, keep tests green -4. Run the targeted E2E spec when done and report that full verification is - required as a separate planner assignment +4. Run the targeted E2E spec when done and report that final change-aware + verification is required as a separate planner assignment diff --git a/.agents/skills/fix/SKILL.md b/.agents/skills/fix/SKILL.md index 2b510a3fa4..fe6b602b6c 100644 --- a/.agents/skills/fix/SKILL.md +++ b/.agents/skills/fix/SKILL.md @@ -9,24 +9,41 @@ Systematic bug fixing: reproduce the problem, find the root cause, apply a minim ## Planner Entry -In the user-started primary session: +For small, clear bugs, the planner may perform the bounded reproduction, TDD +fix, and focused checks directly. Delegate broad diagnosis, large fixes, or +independent work. In the user-started primary session: 1. Delegate reproduction and root-cause diagnosis to an `implementer` worker - with production edits forbidden. + with production edits forbidden. It may create or update only the minimal + failing regression test when that test path is explicitly owned. This + diagnosis packet explicitly overrides the generic implementer workflow: + reproduce and trace, return evidence and root cause, do not patch production + code, and do not continue to Phase 3. 2. Review the evidence and present the root cause to the user. -3. Delegate the regression test and minimal patch to an `implementer` worker. -4. Delegate QA and full verification to their named workers. +3. Delegate the minimal production patch to an `implementer` worker. It + preserves or completes the diagnosis regression test and runs green targeted + verification without duplicating that test unnecessarily. +4. Apply `/planner-orchestration` risk routing: for PR delivery, obtain + qualifying current-head PR AI semantic evidence, always run final `verify`, + and use local QA/review/security agents only for exceptional routes. Stop after dispatching and coordinating these assignments. Do not continue into the direct fix procedure below. A diagnostic worker performs phases 0-2 and -returns evidence. A fix implementer performs phase 3 plus targeted tests. No -worker performs planner phases 4-5 or spawns other workers. +returns evidence and root cause. It returns a red-test result when it owns a +test path; otherwise it returns concrete reproduction evidence and a proposed +regression-test scenario/path. Production code remains forbidden, and +diagnostic instrumentation is test-only when owned or non-mutating. It must +not continue to Phase 3. A fix implementer performs the minimal production +patch, preserves or completes that regression test without unnecessary +duplication, and runs targeted tests. No worker performs planner phases 4-5 or +spawns other workers. ## Available skills and subagents - **`/tdd`** — Use for implementing the fix with a regression test (Red-Green-Refactor). - **`/e2e`** — Use when the bug is in a user-facing flow and needs a Playwright regression test. -- **`/verify`** — The planner launches this after the fix worker reports targeted checks green. +- **`/verify`** — After targeted checks pass, the planner commits through hooks + and launches this post-commit gate before push. - **`/record`** — The planner uses this when the worker reports a durable architectural decision. ## What a fix produces (and what it doesn't) @@ -50,7 +67,8 @@ Create these tasks immediately (use your task/todo tracking tool if available): 1. **Reproduce the bug** — Write a test or find a reliable reproduction case 2. **Find the root cause** — Trace the code path, narrow the scope, state the cause clearly 3. **Fix with TDD** — Minimal fix with regression test, no surrounding refactors -4. **Verify** — Planner delegates full verification and assigns any remediation +4. **Review and verify** — Planner obtains PR-first semantic evidence, + mandatory final verification, and only exceptional local gates; assigns remediation 5. **Record** — Planner updates durable artifacts when worker evidence exposes a requirement or architecture gap Then start with task 0 when the bug is issue-sourced, otherwise task 1. Mark each task in_progress when you begin it and completed when you finish it. Do not skip ahead — fixing without reading the source issue (when one exists) or reproducing leads to patches that don't address the real problem. Fixing without understanding the root cause leads to whack-a-mole. @@ -80,14 +98,19 @@ Mark task 0 as completed. Mark task 1 as in_progress. -Before anything else, reproduce the bug reliably. Pick the right method based on where the bug lives: +Before anything else, reproduce the bug reliably: -- **Backend** (API, logic, data): write a Go test that calls the function/endpoint and asserts the wrong behavior. Run with `go test -v -run TestName ./path/...` -- **Frontend** (UI, state, interaction): write a Playwright E2E test using `/e2e` that navigates to the page and triggers the bug. Run with `make test-e2e` to verify. -- **Full-stack** (user flow breaks end-to-end): Playwright E2E test that exercises the full path from UI through API to DB and back. -- **Unclear where it lives**: start by reading the code path from the reported symptom (a page, an error message, a wrong value) back to its source. Then write the test at the appropriate level. +- **If the diagnosis packet explicitly owns a test path:** write and run the + minimal failing regression test at the appropriate level, then return its red + result with the evidence and root cause. +- **If it does not own a test path:** use existing tests, manual or + non-mutating runtime evidence, or read-only tracing. Do not edit tests or + production. Return concrete reproduction evidence plus the proposed + regression-test scenario and path for the Phase 3 patch worker. -If it can't be reproduced, add logging/assertions to gather more info — don't guess at a fix. +If it can't be reproduced, use test-only assertions/instrumentation only when +the test path is owned; otherwise use non-mutating runtime evidence — don't +guess at a fix. Find the minimal reproduction case: strip away everything that isn't needed to trigger the bug. For flaky Playwright failures, do not stop after one clean focused run. Use the @@ -107,7 +130,11 @@ Mark task 2 as in_progress. Don't guess and patch — systematically narrow the scope: -**Trace the code path:** Follow the data from input to the failure point. Add assertions or logging at the midpoint of the call chain. Is the data correct there? If yes, the bug is downstream. If no, upstream. Repeat until you find the exact line where things go wrong. +**Trace the code path:** Follow the data from input to the failure point. Use +test-only assertions/instrumentation only when the test path is owned; +otherwise use non-mutating runtime evidence at the midpoint of the call chain. +Is the data correct there? If yes, the bug is downstream. If no, upstream. +Repeat until you find the exact line where things go wrong. **Narrow the input:** What's the smallest input that triggers the bug? What's the largest input that succeeds? Strip away everything that isn't needed to trigger it. @@ -129,9 +156,11 @@ Mark task 2 as completed. Mark task 3 as in_progress. Follow `/tdd`: -1. Write a test that reproduces the exact bug — confirm it fails +1. Preserve or complete the diagnosis regression test; if diagnosis did not + own a test path, add the proposed regression test, then confirm the exact + bug fails 2. Write the minimal fix — change only what's necessary, don't refactor surrounding code -3. Confirm the test passes and no other tests regress +3. Confirm the regression test and targeted verification pass Mark task 3 as completed. @@ -141,9 +170,30 @@ Mark task 3 as completed. Mark task 4 as in_progress. -This is a planner coordination phase. Launch the registered `verify` worker, -review its report, and assign any failures to a new implementer. The fix worker -only reports its targeted test results and any similar patterns it noticed. +This is a planner coordination phase. After the fix implementer reports its +targeted test results and compact handoff capsule (intent/acceptance; base/head +SHA when applicable; changed files and entry points; named spec/ADR sections; +risk tags; exact targeted commands/results; uncertainties), apply +`/planner-orchestration` risk routing. For PR delivery, defer routine semantic +review to qualifying exact-current-head PR AI evidence; do not launch local +`code-review` by default. Use `qa` only for unusually large/complex +multi-component behavior or an important integration boundary without faithful +tests. Use `security-auditor` only for high-impact new/changed authz, +workspace-isolation, secrets, untrusted-execution, or credential-trust +boundaries, an explicit request, or concrete automated security concerns. +Commit the accepted fix through `/commit`, then always run final `verify` +before push with its hook receipt. The planner's acceptance check is not a +substitute for the required evidence. + +Route every finding to a bounded implementer packet. Reuse the same native +thread when role, change, and file scope remain materially the same; use a new +thread after major redesign, unrelated scope, stale/noisy context, or when +independent judgment is needed. Rerun only affected semantic, QA, and security +gates only when the remediation meets their exceptional route. For a small, +scope-preserving PR fix, use the finding, focused regression, final Spark +`verify`, and fresh qualifying exact-head PR AI review. The fix worker only +reports targeted results, its handoff capsule, and any similar patterns it +noticed. Mark task 4 as completed. diff --git a/.agents/skills/harness-improvement/SKILL.md b/.agents/skills/harness-improvement/SKILL.md index 9f11487f2b..762e57bf8e 100644 --- a/.agents/skills/harness-improvement/SKILL.md +++ b/.agents/skills/harness-improvement/SKILL.md @@ -9,10 +9,9 @@ Use this skill to turn lessons from real agent sessions into durable harness cha ## Planner Entry -In a user-started primary session, inventory and -plan the harness change, then delegate file edits and validation to a native -`implementer` subagent. Do not use Kandev MCP task/session APIs to launch that -worker. An explicitly assigned worker continues below and does not spawn agents. +The planner may inventory, edit, and validate a small localized harness change +directly. Delegate broad cross-platform migration or independent work when it +has positive ROI. Do not use Kandev MCP task/session APIs for workers. ## Choose The Artifact diff --git a/.agents/skills/harness-improvement/references/agents.md b/.agents/skills/harness-improvement/references/agents.md index 57cbfe13c8..813f2c7761 100644 --- a/.agents/skills/harness-improvement/references/agents.md +++ b/.agents/skills/harness-improvement/references/agents.md @@ -42,18 +42,40 @@ Guidelines: - `tools`: smallest sufficient set. - `model`: use the current role tier. Claude uses `opus` for architecture/security/deep review and `sonnet` for other workers. Codex uses - GPT-5.6 Sol, Terra, and Luna for frontier, balanced, and cheap roles. Cursor - uses Grok 4.5 for frontier review and Composer 2.5 for normal workers. + GPT-5.6 Sol, Terra, and Luna for frontier, balanced, and cheap roles. Codex + may additionally use GPT-5.3-Codex-Spark as an opt-in specialist tier, not + a replacement for those tiers: read-only bounded exploration or explicit + localized low-risk implementation. Codex `verify` is the sole exception: + pin it to Spark/medium for deterministic mechanical verification, while + `pr-poller` remains Luna/low. Spark is not for polling, architecture, + security, or deep review. Cursor uses Grok 4.5 for frontier review and + Composer 2.5 for normal workers. OpenCode intentionally inherits the configured provider in this repository. - `effort`: reasoning depth for Claude source agents; map to Codex `model_reasoning_effort` where supported. Kandev role mapping: - `high`: `architect`, `code-review`, `security-auditor` - `medium`: `implementer`, `test-engineer`, `qa`, `simplify` - `low`: `verify`, `pr-poller` + + The list above remains the semantic tier mapping for non-Codex platforms. + Codex alone pins its existing `verify` agent to GPT-5.3-Codex-Spark at medium + effort; `pr-poller` remains Luna/low. - `permissionMode`: read-only/review roles should not accept edits; implementers can accept edits. - `skills`: preload only the minimum domain skills needed. - Body: input required, workflow, output contract, stop conditions, and "Do not spawn subagents" unless nested delegation is intentional. +Codex-specific Spark roles: + +- `spark-explorer`: GPT-5.3-Codex-Spark, medium effort, read-only. Limit it to + bounded exploration, call-path tracing, and evidence gathering; explicitly + forbid architecture, security, final review, implementation, and edits. +- `spark-implementer`: GPT-5.3-Codex-Spark, medium effort, workspace-write. + Limit it to targeted UI changes or localized low-risk edits with explicit + acceptance and exact verification. Require tests/verification and stop for + ambiguity, cross-subsystem work, auth/security/workspace isolation, + concurrency, persistence/migrations, process/filesystem execution, public + contracts, or architectural changes. + ## Platform Choice Delegation is always native to the active harness: diff --git a/.agents/skills/harness-improvement/references/platforms/cursor.md b/.agents/skills/harness-improvement/references/platforms/cursor.md index d4f1f076a3..69d55ad9a0 100644 --- a/.agents/skills/harness-improvement/references/platforms/cursor.md +++ b/.agents/skills/harness-improvement/references/platforms/cursor.md @@ -10,7 +10,7 @@ Sources: ## Skills -Cursor supports Agent Skills as reusable `SKILL.md` packages. Project skills are generated under `.cursor/skills/` by Cursor's migration flow, and skills follow the standard folder shape: +Cursor supports Agent Skills as reusable `SKILL.md` packages. It discovers project skills from both `.cursor/skills/` and the shared `.agents/skills/` source Kandev uses directly; no `.cursor/skills` mirror or symlink is required. Cursor's migration flow can also generate project skills under `.cursor/skills/`, which follows the standard folder shape: ```text .cursor/skills//SKILL.md diff --git a/.agents/skills/planner-orchestration/SKILL.md b/.agents/skills/planner-orchestration/SKILL.md index 48a7db2c6a..2bbc9c578f 100644 --- a/.agents/skills/planner-orchestration/SKILL.md +++ b/.agents/skills/planner-orchestration/SKILL.md @@ -5,8 +5,8 @@ description: Enforce Kandev's planner-and-worker execution model. Use in the use # Planner Orchestration -The user-started primary session is the **planner**. A custom subagent launched -through the current harness's native delegation tool is a **worker**. +The user-started primary session is the **planner** and default architect. A +custom subagent launched through native delegation is a **worker**. ## Planner Contract @@ -19,16 +19,22 @@ The planner may: - Spawn workers, monitor results, review diffs and reports, and communicate status to the user. -The planner must not: +The planner may directly do small scoped work: one clear concern, a few +localized files, no meaningful isolation/parallelism benefit, and quick bounded +verification. This includes small code/test/harness/docs edits, focused checks, +routine Git/GitHub steps, PR-thread triage/replies, and small PR fixups. Apply +the relevant skill/TDD, preserve dirty-worktree safety, and use final delegated +Spark `verify` after commit and before push for code, tests, or config. Supply +the `/commit` hook receipt so changed-scope verification avoids only proven +duplicate hook work. -- Edit application code, tests, generated files, package metadata, or CI. -- Run implementation, test, formatting, lint, build, commit, push, or PR - commands. -- Resolve implementation conflicts or fix worker output directly. -- Continue as an implementation fallback when delegation is unavailable. - -If no suitable worker can be launched, stop and tell the user which delegated -capability is unavailable. Do not silently become the worker. +Delegate when ROI or independent evidence matters: broad/unknown exploration, +substantial plan work, large/cross-component changes, parallel packets, +long/noisy E2E or debugging, exceptional specialists, and final change-aware +`verify`. Keep long monitoring with cheap `pr-poller`. Estimate context reload and +coordination cost; delegation is not default ceremony. Reuse a worker thread +for related followups. Architect is user-requested independent second opinion, +not an automatic planner step. ## Native Delegation Only @@ -63,7 +69,7 @@ Every delegated assignment must contain: ```text Role: + simplify | verify | pr-poller | spark-explorer | spark-implementer> Objective: Inputs: Owned files: @@ -81,6 +87,12 @@ Constraints: - Do not spawn subagents. ``` +Pass a compact **handoff capsule** to a follow-up worker instead of the parent +transcript or pasted full specs. Include only: intent/acceptance; base and head +SHA when applicable; changed files and entry points; named spec/ADR sections; +risk tags; exact targeted commands/results; and uncertainties. Link the paths +and named sections the worker must read. + Workers share a mutable checkout unless the runtime explicitly provides an isolated worktree. Run them sequentially by default. Parallelize only when their owned files do not overlap and neither worker changes shared schemas, @@ -94,16 +106,84 @@ The planner inherits the strong model selected by the user. Platform mirrors pin workers by role: - Balanced workers: implementation, tests, QA, simplification. -- Cheap workers: polling and mechanical verification where the platform has a - suitable lower-cost model. +- Cheap workers: polling, including Codex `pr-poller` on Luna/low. +- Codex `verify` is pinned to GPT-5.3-Codex-Spark/medium for deterministic + mechanical verification; this uses Spark's separate quota while + `pr-poller` remains the cheap polling role. - Frontier workers: architecture, security, and deep code review only. +- Spark is a Codex-only, opt-in specialist tier: use `spark-explorer` for + bounded read-only evidence gathering and `spark-implementer` only for + explicit, localized low-risk UI or code edits. It does not replace the + balanced, frontier, or cheap tiers outside the Codex `verify` exception, and + is not for polling, architecture, security, or deep review. Do not omit a worker model when omission inherits the planner's model, except for OpenCode where this repository intentionally keeps provider choice inherited. -## Completion +## Evidence, Risk Routing, And Completion + +The planner derives risk tags from the union of worker reports and its own +inspection. Use `localized`, `user-flow`, `integration`, `public-contract`, +`persistence`, `concurrency`, `security`, and `architecture`. An uncertainty +uses the stronger applicable gate. + +| Evidence or risk | Required route | +| --- | --- | +| Routine work, including ordinary integration with faithful targeted tests | Qualifying PR AI semantic evidence and final `verify` when PR delivery is in scope. | +| Unusually large/complex multi-component behavior or an important integration boundary lacking faithful tests | Add `qa`, semantic evidence, and final `verify`. | +| Unusually large/complex/cross-cutting architecture; terminal readiness cannot wait for contradictory or repeatedly unavailable PR evidence; explicit request; or a deep automated-review gap | Add local frontier `code-review`. | +| High-impact new/changed authz, workspace-isolation, secrets, untrusted-execution, or credential-trust boundary; explicit request; or concrete automated security concern | Add `security-auditor`, applicable review/QA, and final `verify`. | + +When PR delivery is in scope, one qualifying automated AI PR review is the +default semantic evidence; defer routine local `code-review` until that review. +Use local review only for the exceptional routes above. A qualifying review names the +selected/configured reviewer; has known, complete review and head evidence with +matching PR-view checks/opening/closing SHA and a complete check snapshot; has +an API review `commit_id` exactly equal to the stable evidence head (timestamps +never prove head coverage); is an APPROVED +review without a blocker signal or a COMMENTED review with the explicit +`` terminal marker; covers changed code and tests; +for the configured dedicated `${OPENCODE_REVIEW_APP_SLUG}[bot]` OpenCode App, require emitted `trusted_producer=true` on that COMMENTED exact-head record; its raw predicate binds the dedicated-App marker and fixed workflow/run-attempt API, and displayed check names never qualify, so unrelated Actions reviews cannot qualify; +other explicitly selected reviewers retain the generic exact-head eligibility and blocker gates without App provenance; +and has no active `CHANGES_REQUESTED`, unresolved blocker, actionable review +thread, or blocked exact-current-head review from any author. Dismissed, +pending, unknown, blocked, or ambiguous commented reviews never qualify; nor +does arbitrary bot prose. Do not wait for every configured bot, but do not +ignore any actionable finding that has arrived. A head change makes prior PR +review stale. Without PR delivery, use local review only when semantic review +is needed by the exceptional routes above. + +`qa` is exceptional, not a consequence of an ordinary integration tag. Use it +only for unusually large/complex multi-component behavior or important +integration behavior without faithful tests. A routine fix preserving an +existing security boundary does not require `security-auditor`. The planner accepts a worker result only after checking that its file scope, acceptance criteria, and reported verification match the assignment. Any -follow-up fix is a new worker assignment, not planner execution. +follow-up remains a bounded worker assignment, not planner execution. Reuse +the same native agent thread for remediation or re-review when role, change, +and file scope remain materially the same. Spawn a fresh thread after a major +redesign, unrelated scope, stale/noisy context, or when independent judgment +is specifically needed. + +The planner's acceptance check is not a substitute for a required quality +gate. Keep responsibilities separate: + +- The planner checks assignment scope, acceptance criteria, and evidence. +- `code-review` or qualifying PR AI evidence performs semantic diff review. +- `qa` challenges integrated behavior and edge cases when risk requires it. +- `verify` mechanically runs required uncovered format, typecheck, test, and + lint commands on the committed artifact; eligible hook evidence may replace + exact changed-file format/lint duplicates only. + +For every final remediation affecting code, tests, or config, commit first, +then final `verify` before push. A small, scope-preserving PR remediation uses +the reviewer +finding, focused tests, final Spark `verify`, and fresh qualifying exact-head +automated review; it almost never relaunches QA, code-review, or +security-auditor just because original tags remain. Relaunch one only if the +fix becomes large/complex, changes a contract or trust boundary, invalidates +prior evidence, or exposes a gap that gate must assess. A bug fix preserving +an existing ADR or invariant is not by itself a new boundary. Simplification, +when used, happens before semantic review so its edits are covered. diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index d9673dfacc..6663384f32 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -9,12 +9,10 @@ Wait for CI and code review to complete on a pull request, fix any failures or v ## Planner Entry -The user-started primary session coordinates this -workflow by delegating polling to `pr-poller`, code/comment remediation to an -`implementer`, full checks to `verify`, and commit/push actions to a final -bounded implementer assignment. It does not run GitHub, test, edit, commit, or -push commands directly. If a required worker is unavailable, stop and report -the blocked phase. +The planner keeps long polling on cheap `pr-poller` and final change-aware +checks on Spark `verify`, but may directly triage/reply to threads and make a small +scope-preserving fixup with focused checks. Delegate broad remediation or noisy +work when it has positive ROI. Each worker uses only its assigned phase: polling, remediation, verification, or delivery. Workers never invoke one another. @@ -25,8 +23,8 @@ If `pr-poller` recommends `GitHub access requires approval; planner must surface ## Available skills and subagents -- **`pr-poller` worker** — Polls CI checks and the 5 review bots until terminal, returns a compact structured report. -- **`verify` worker** — Runs the full verification pipeline and returns a compact pass/fail report before delivery. +- **`pr-poller` worker** — Polls CI/review state and can use one selected reviewer's exact-head evidence without waiting for unrelated bots. +- **`verify` worker** — Runs change-aware verification and returns a compact pass/fail report before delivery. - **`/e2e`** — Read for debugging guidance when E2E tests fail in CI. Covers test patterns, run commands, failure triage, and local reproduction. - **`/commit`** — Use for staging and committing fixes with Conventional Commits format. @@ -47,7 +45,7 @@ requests task tracking: 2. **Fix failing CI checks** — Read failing run logs (via `scripts/run-quiet gh-run -- gh run view ...`), fix issues, run E2E tests locally if needed 3. **Triage review comments** — Classify each comment as valid, already addressed, nitpick, or wrong 4. **Address each comment** — Fix or reply with reasoning, resolve threads -5. **Verify, commit, push** — Delegate verification, then separate bounded commit/push delivery +5. **Commit, verify, push** — Commit with hooks, delegate hook-aware verification, then push 6. **Re-check** — Delegate to `pr-poller` again; if new failures, loop back to task 2 7. **Summary** — Report what was done @@ -61,9 +59,17 @@ Then start with task 1. Mark each task in_progress when you begin it and complet Mark task 1 as in_progress. -The planner invokes the registered `pr-poller` with the PR number. The worker: +The planner invokes the registered `pr-poller` with the PR number and, when a +reviewer is designated for semantic evidence, `selected_reviewer=`. +For routine same-repository OpenCode evidence, select +`${OPENCODE_REVIEW_APP_SLUG}[bot]` only when emitted `trusted_producer=true` +and external App configuration (enablement, client ID, slug, protected environment, +environment-only private key, current-repository-only installation, and pull-request write permission) are present. Administrators must create `OPENCODE_REVIEW_ENV_APP_PRIVATE_KEY` only in the protected `opencode-review-trusted` environment, never as a repository/org fallback secret, and restrict environment deployment branches to the protected default branch. Pass that login +to `scripts/pr-state --trusted-reviewer`; otherwise use normal polling or local +frontier review. Never use this shortcut for forks, security, or architecture. +The worker: - Fetches the current CI/bot/comment state once -- Polls (30s cadence, **20 min cap**) until every CI check and every bot (CodeRabbit, Greptile, Claude, OpenCode, cubic) reaches a terminal state +- Polls (30s cadence, **20 min cap**) until CI and configured-reviewer evidence reach a terminal state, unless selected exact-head evidence is qualified or blocked after CI completes - Counts unresolved review threads and bot issue comments - Returns a structured report between `=== pr-poller report ===` and `=== end ===` markers @@ -91,11 +97,28 @@ above overrides this rule. The planner does not inspect or resolve conflicts dir - `bots.` — `done` / `rate_limited` / `pending` / `timeout` / `unknown`. Only `done` and `rate_limited` are clean; surface `timeout`, and treat `pending` or `unknown` as incomplete state. -- `unresolved_review_threads` and `issue_comments_from_bots` — drive steps 3-4. +- `review_evidence` — contains `reviewer`, `qualification`, `reviewed_head_sha`, + `review_state`, `eligibility`, `verdict`, `active_changes_requested`, and + `blocked_exact_current_head_reviews`. + With a selected reviewer, `qualified` or `blocked` may end polling after CI; + `unqualified`/`unknown` requires normal polling or local-review fallback. An + active changes request, actionable unresolved thread, or nonzero exact-head + blocked-review count always blocks. + Qualification needs matching PR-view checks/opening/closing heads plus an API + `commit_id` equal to `evidence_head_sha`, `checks_snapshot_complete=true`, + never timestamp-only inference. If check snapshot H1 and reviews are H2, + treat combined evidence as unknown and retry/fall back. Only approved + reviews without blocker signals or COMMENTED reviews with the explicit `` + marker are eligible. For the configured dedicated OpenCode App + producer, require emitted `trusted_producer=true`; displayed check names never qualify. + Generic Actions reviews are never sufficient. Dismissed, pending, unknown, and ambiguous comments are + not. +- `unresolved_review_threads` and `actionable_issue_comments_from_bots` — drive steps 3-4. Skip to step 5 only when both are known `0`, `ci_failed` is explicitly known empty, `ci_pending` is `none`, the merge-conflict gate is known clean, and - every bot is `done` or `rate_limited` (still run verify + push if you have - fixes from earlier). + every configured reviewer is done or `rate_limited` (or an explicitly selected reviewer is + `qualified` with no blockers; still run verify + push if you have fixes from + earlier). **E2E CI outlasts the poller.** The pr-poller caps at ~20 minutes. Standard E2E shards plus container shards often run longer. GitHub can expand E2E matrix jobs late, and the shard matrix may appear only after the build job finishes; if pending checks briefly drop near zero and then jump when E2E shards appear, keep treating that as normal pending CI unless a shard reports failure. If the report shows `ci_pending` with only E2E/lint jobs and `ci_failed` is known empty, re-invoke pr-poller once those jobs finish — do not spin a manual `gh pr checks` loop in the parent. If the cap hits with E2E still pending, report "CI in progress" to the user instead of blocking, and include the exact pending shard names from `ci_pending`. @@ -103,81 +126,13 @@ above overrides this rule. The planner does not inspect or resolve conflicts dir #### Polling Worker Reference -The commands below document what the registered `pr-poller` gathers. They are -not a planner or remediation-worker fallback. If `pr-poller` cannot be launched, -stop and report the blocked phase. - -```bash -scripts/pr-state --summary -``` - -Request runtime network approval before the first GitHub helper call; denial, cancellation, or interruption stops the workflow, while transient failures from approved commands use bounded retry. - -`scripts/pr-state` accepts flags before or after the PR (`scripts/pr-state --summary ` and `scripts/pr-state --summary` both work). When parsing output with `jq`, prefer writing the JSON to a temp file first, then running `jq` against that file; this avoids `set -e` surprises and prevents stderr from corrupting a JSON pipe. - -Prefer `scripts/pr-state --summary ` for direct polling and `scripts/pr-resolve list ` for review-thread state over raw `gh pr checks`. `gh pr checks` only reports CI/status checks; review bots can open unresolved threads after CI is green, and a checks-only poll will miss that blocker. - -By default, `scripts/pr-state` returns comments, reviews, and review threads created after the latest PR head commit only. This is intentional for fixup passes: it keeps old bot summaries and already-addressed historical threads out of the working set. Use `scripts/pr-state --summary --all ` only when you need to audit the full PR history. - -The summary output contains: - -- `failed_checks` — actionable non-green checks with `name`, `workflow`, `status`, `conclusion`, `run_id`, `job_id`, `details_url`, and `target_url` -- `pending_checks` — still-running or queued checks with `name`, `workflow`, `status`, `run_id`, `job_id`, `details_url`, and `target_url` -- `unresolved_review_thread_count` — total unresolved thread count on the PR, including older unresolved threads outside the current-head filter -- `filtered_review_thread_count` — informational count for the current-head-filtered review-thread view; it can include resolved or historical comments, so do not treat it as a blocker by itself -- `unresolved_threads` — compact current-head inline review state to triage in this fixup pass -- `hidden_unresolved_threads` — compact unresolved threads outside the current-head filter when historical unresolved threads exist; treat these as actionable blockers, not ignorable history -- `errors` — data-gathering failures; treat affected fields as unknown instead of reconstructing them ad hoc - -If `scripts/pr-state --summary ` briefly returns `branch:"unknown"` or reports that `gh pr view` failed while `gh pr view ` works directly, treat it as a transient state-resolution issue. Re-run the explicit PR-number command once and fall back to direct `gh pr view ` / targeted `gh run view` checks for that pass instead of assuming the PR state is unavailable. - -If `scripts/pr-state --summary ` returns `since: null` with `errors` like `repo` or `since`, but still includes valid `failed_checks`, `pending_checks`, and `unresolved_review_thread_count`, use the check/thread data for that poll and rerun once on the next cadence. Do not discard usable CI state just because head-commit metadata lookup failed. - -If `scripts/pr-state --summary ` returns valid `failed_checks` and `pending_checks` but `unresolved_review_thread_count: null` with an error such as `{"source":"review_threads","message":"gh api graphql reviewThreads failed"}`, use the check data for that poll but treat review-thread state as unknown. Do not report review as clean or blocked from that snapshot; rerun `scripts/pr-state --summary ` on the next cadence to recover thread state. Before a final status, retry `scripts/pr-state --summary ` once; if review thread state still errors, run `scripts/pr-resolve list ` directly. Only report "no unresolved review threads" after one of those succeeds. - -If `scripts/pr-state --summary ` fails with `jq: Argument list too long`, do not debug the summary script during fixup. Split the fallback checks: - -```bash -gh pr checks -scripts/pr-resolve list -``` - -If the checks table is long or mixed with request diagnostics, keep only -actionable rows: - -```bash -gh pr checks 2>/dev/null | awk -F '\t' '$2 == "pending" || $2 == "fail" {print}' -scripts/pr-resolve list -``` - -`gh pr checks` gives CI status only; `scripts/pr-resolve list` is still required for unresolved inline review threads. `gh pr checks` can exit nonzero when checks are pending or failed (for example exit code 8) while still printing usable status rows. Treat its table output as valid state, not automatically as a shell/tool failure; parse the rows for `fail`, `pending`, and `pass`, and avoid `set -e` flows unless you intentionally capture or ignore the exit code. - -If `unresolved_review_thread_count` is nonzero but `unresolved_threads` is empty, or if `hidden_unresolved_threads` is non-empty, run `scripts/pr-resolve list ` before acting. Fetch each full body with `scripts/pr-state --comment ` or `scripts/pr-resolve show `; hidden threads can contain valid current blockers even when the current-head filtered view is empty. `pr-state` can briefly report total unresolved count from historical state while current-head unresolved threads are empty; `pr-resolve list` is the authoritative actionable thread set for fixup work. If `scripts/pr-state --summary` reports a nonzero unresolved count but `scripts/pr-resolve show ` says the listed thread is `resolved: true`, treat the summary as stale state. Wait briefly, rerun `scripts/pr-state --summary `, and do not reply again to an already-resolved thread. - -Poll at a 30s cadence with a **20 min cap**. Prefer one-shot `scripts/pr-state --summary ` checks, or a bounded command that can finish naturally, over long inline shell loops. In this environment, a running non-TTY loop may not receive Ctrl-C through `write_stdin`, so avoid `while sleep ...; do ...; done` polling in the main session. Run `sleep 30` and then a fresh `scripts/pr-state --summary ` as separate commands; a combined command can capture blank output. Record wall-clock time around requested sleeps: if less elapsed than requested, treat the wait as interrupted, re-poll, and report the shorter time. Rerun a blank summary before interpreting it. - -Stop early if any required check fails. A `queued` or `in_progress` job caused by GitHub runner capacity is pending CI, not a failure: when the requested watch window or cap ends with no failures or unresolved comments, do not make speculative code changes. Report each pending job's exact name and status, plus its `details_url` or Actions run URL. If the user explicitly asks to poll until CI is green, continue past the normal pending-E2E stopping point with bounded one-shot `sleep N; scripts/pr-state --summary ` checks; stop immediately on any `failed_checks`, and continue until `failed_checks: []`, `pending_checks: []`, and `unresolved_review_thread_count: 0`. Do not run `gh pr checks --watch` in the main session unless the runtime can keep the watcher isolated and automatically clean it up. If you do use `gh pr checks --watch`, keep watching until the command exits; GitHub can expand matrix jobs after an initial aggregate "Build" check passes, so the first green build/lint/test rows are not necessarily terminal. - -For E2E-only pending phases, summarize counts before dumping long shard lists into context: - -```bash -scripts/pr-state --summary > /tmp/prstate-.json -jq '{failed_checks, pending_count:(.pending_checks|length), pending_by_workflow:(.pending_checks|group_by(.workflow)|map({workflow:.[0].workflow,count:length,statuses:(map(.status)|unique)})), unresolved_review_thread_count, errors}' /tmp/prstate-.json -``` - -When stopping at the cap, print the exact pending names from the saved snapshot: - -```bash -jq -r '.pending_checks[] | "\(.status) | \(.name)"' /tmp/prstate-.json -``` - -If a user interrupts a long manual poll loop (`sleep`, `gh pr checks`, or `scripts/pr-state`), check for leftover polling processes before switching tasks and terminate only the processes you started. - -Use raw mode only when debugging an odd GitHub state: - -```bash -scripts/pr-state -``` +Normal routing uses only the compact `pr-poller` report above. Load +[review-evidence.md](references/review-evidence.md) only when that worker is +unavailable, its report is incomplete/contradictory, or raw `pr-state` evidence +must be interpreted during a PR-state incident. It documents bounded polling, +raw field semantics, and safe fallback diagnostics; it is not a planner or +remediation-worker polling fallback. If `pr-poller` cannot be launched, stop +and report the blocked phase. Mark task 1 as completed. @@ -185,9 +140,10 @@ Mark task 1 as completed. Mark task 2 as in_progress. -The planner assigns this section to a remediation implementer with the poller -report, failed run IDs, owned files, and acceptance criteria. The implementer -does not invoke other workers. +The planner may handle a small, localized CI fix directly. Delegate broad, +cross-component, or noisy remediation with the poller report, failed run IDs, +owned files, and acceptance criteria. An assigned implementer does not invoke +other workers. **Sanity-check the poller's `ci_failed` before fixing anything.** Confirm each reported check `name` actually appears in `gh pr checks ` output and its `run_id` resolves (`gh run view ` must not 404). If the report cites checks the repo doesn't have, discard it and re-gather state directly before touching code. @@ -235,10 +191,11 @@ Mark task 2 as completed. Mark task 3 as in_progress. -The planner assigns this section to a remediation implementer with the compact -thread list and relevant diff scope. +The planner may triage a small thread set directly. Delegate only when the +thread set or required investigation is broad enough to justify a fresh +context, supplying the compact thread list and relevant diff scope. -Use the report's `unresolved_review_threads` and `issue_comments_from_bots` counts to know whether there's anything to triage. If both are 0, mark this step completed and move on. +Use the report's `unresolved_review_threads` and `actionable_issue_comments_from_bots` counts to know whether there's anything to triage. If both are 0, mark this step completed and move on. Otherwise, fetch the actual comment bodies on demand — one bot or one set at a time, not all at once: @@ -310,8 +267,17 @@ Mark task 3 as completed. Mark task 4 as in_progress. -The same bounded remediation implementer addresses only the assigned threads -and reports its edits, replies, and targeted checks. +The planner or assigned remediation implementer addresses only the selected +threads and records edits, replies, and targeted checks. + +For a small remediation that preserves the established scope and boundary, use +the reviewer finding, focused regression, final Spark `verify`, and a fresh +qualifying exact-head automated review. Retain completed QA, local review, and +security evidence: original risk tags alone do not require relaunching them. +Almost never relaunch a named gate; do so only if the fix becomes large/complex, +changes a contract or trust boundary, invalidates prior evidence, or exposes a +gap that gate must assess. A bug fix preserving an existing ADR or invariant is +not by itself a new boundary. Every comment must get a response — either a fix or a reply explaining why it was skipped. @@ -424,17 +390,21 @@ Informational threads (acknowledged, no code change) still need `scripts/pr-reso Mark task 4 as completed. -### 5. Verify, commit, and push +### 5. Commit, verify, and push Mark task 5 as in_progress. -1. The planner launches the registered `verify` worker. It reports failures - without changing source/test logic. -2. If verification fails, the planner creates a new bounded remediation packet, - then launches a fresh verification assignment. Do not deliver until green. -3. After a green report, the planner delegates commit and push as bounded - delivery assignments using `/commit` and `/push`. Supply the verification - result and exact intended paths. Workers do not invoke one another. +1. The planner commits the focused fix through `/commit` and preserves its hook + receipt plus the previously verified/pushed head as verification scope base. +2. The planner launches registered `verify` on that committed artifact with + the receipt. It reports failures without changing source/test logic and + avoids only eligible duplicate hook-covered checks. +3. If verification fails or changes formatting, the planner handles a small + remediation directly or creates a bounded packet, then commits and launches + fresh verification. Do not push until green. +4. After a green report, the planner normally pushes directly using `/push`. + Delegate delivery only when isolation or coordination provides positive ROI. + Workers do not invoke one another. Mark task 5 as completed after the delivery worker reports the pushed commit. @@ -452,14 +422,19 @@ launched, stop and report the blocked phase. Parse the returned report: `merge_state_status` is known and not `DIRTY`, `local_unmerged_entries: 0`, the complete report establishes `ci_failed` as explicitly known empty, `ci_pending: none`, `unresolved_review_threads: 0`, - `issue_comments_from_bots: 0`, and every bot row is `done` or - `rate_limited`. + `actionable_issue_comments_from_bots: 0`, and every configured-reviewer row is done or + `rate_limited`; an explicit selected reviewer may replace the last condition + only with known, exact-current-head `qualification=qualified`, no active + changes request or actionable unresolved thread, and a zero exact-head + blocked-review count. Only the dedicated OpenCode App additionally requires `trusted_producer=true`; other explicitly selected reviewers retain generic qualification without App provenance. - Treat a clean intermediate state as provisional while any mergeability or local-conflict field is `unknown`, `ci_failed: unknown` or any other result - is not explicitly known empty, `ci_pending` is not `none`, or any bot row is - `pending`, `timeout`, or `unknown`. -- If only long-running checks remain in `ci_pending` after full local - verification is green, `ci_failed` is explicitly known empty, and + is not explicitly known empty, or `ci_pending` is not `none`. Unrelated bot + rows may remain `pending`, `timeout`, or `unknown` after an explicit selected + reviewer is exact-head, complete, terminal/qualified, and all arrived + findings are clean; otherwise preserve the normal configured-reviewer provisional gate. +- If only long-running checks remain in `ci_pending` after the required local + verification scope is green, `ci_failed` is explicitly known empty, and `unresolved_review_threads: 0`, stop at the bounded cap and report the exact pending checks. Continue immediately if a check fails or a new thread appears. - If new CI failures appeared from the latest commit → loop back to task 2 and reset task 2-5 to `in_progress` as needed. diff --git a/.agents/skills/pr-fixup/references/review-evidence.md b/.agents/skills/pr-fixup/references/review-evidence.md new file mode 100644 index 0000000000..2f871bbfec --- /dev/null +++ b/.agents/skills/pr-fixup/references/review-evidence.md @@ -0,0 +1,87 @@ +# Poller Review-Evidence Mechanics + +Load this reference only when the `pr-poller` is unavailable, its report is +incomplete or contradictory, or a planner needs to interpret raw +`scripts/pr-state` output while resolving a PR-state incident. It does not +authorize the planner or a remediation worker to replace the registered +`pr-poller` during normal operation. + +Request runtime network approval before the first GitHub helper call; denial, +cancellation, or interruption stops the workflow. Retry transient failures from +approved commands only within the bounded polling cadence. + +Use `scripts/pr-state --summary ` for CI/review state and +`scripts/pr-resolve list ` for review-thread state. `scripts/pr-state` +accepts flags before or after the PR; when parsing with `jq`, save JSON to a +temp file first so stderr does not corrupt the pipe. Default state is limited to +items after the latest head commit; use `--summary --all` only for a deliberate +historical audit. + +The summary fields are: + +- `failed_checks` and `pending_checks`: actionable check state and run/job URLs. +- `unresolved_review_thread_count`: all unresolved threads; use + `hidden_unresolved_threads` as blockers even outside the current-head filter. +- `review_evidence`, `checks_head_sha`, and `checks_snapshot_complete`: exact + head evidence. A head race or incomplete fetch is unknown, never inferred + from timestamps. +- `errors`: affected data is unknown; do not reconstruct it from memory. + +If head metadata fails but check/thread data is usable, use that data only for +the current poll and retry once at the next cadence. If review-thread state is +unknown, do not call review clean/blocked; retry once, then use +`scripts/pr-resolve list ` before a final status. If total unresolved count +is nonzero while visible threads are empty, fetch the authoritative thread list +and full bodies with `scripts/pr-state --comment ` or +`scripts/pr-resolve show `. + +If `branch:"unknown"` or PR-view resolution is transient, retry the explicit +PR-number command once before using direct targeted GitHub fallback. Do not +discard valid check/thread data merely because `since`/repository metadata +failed; however, an unknown review-thread count is never clean evidence. + +If `pr-state` hits an argument-list limit, fall back to: + +```bash +gh pr checks +scripts/pr-resolve list +``` + +`gh pr checks` covers CI only and can return nonzero while still printing usable +pending/failing rows. Keep only actionable rows and retain `pr-resolve` for +threads: + +```bash +checks_file=/tmp/pr-checks-.txt +if gh pr checks >"$checks_file" 2>"${checks_file}.err"; then status=0; else status=$?; fi +printf 'gh pr checks exit=%s\n' "$status" +cat "$checks_file" +test -s "${checks_file}.err" && cat "${checks_file}.err" >&2 +awk -F '\t' '$2 == "pending" || $2 == "fail" {print}' "$checks_file" +scripts/pr-resolve list +``` + +Transport/collection failure leaves checks unknown. Parseable pending/failing +rows remain usable when `gh pr checks` exits 8; never hide diagnostics in a pipe. + +When `hidden_unresolved_threads` exists or total unresolved count exceeds the +visible list, fetch each body with `scripts/pr-state --comment ` or +`scripts/pr-resolve show `. A listed thread that +is already resolved is stale summary state: re-poll and do not reply again. + +Poll at 30-second cadence with a 20-minute cap using bounded one-shot commands; +avoid long inline loops and `gh pr checks --watch`. Stop early on a required +failure. Queued/in-progress jobs are pending, not speculative-fix triggers. On +an explicit wait-through-CI request, repeat bounded checks until failures, +pending checks, and unresolved-thread count are all empty/zero. + +For E2E-only pending work, summarize a saved snapshot before printing shards: + +```bash +scripts/pr-state --summary > /tmp/prstate-.json +jq '{failed_checks, pending_count:(.pending_checks|length), unresolved_review_thread_count, errors}' /tmp/prstate-.json +jq -r '.pending_checks[] | "\(.status) | \(.name)"' /tmp/prstate-.json +``` + +If a manual poll is interrupted, terminate only polling processes you started. +Use raw `scripts/pr-state ` only for an odd-state diagnostic. diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index b5ac3eb24c..03b4d637f4 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -1,16 +1,15 @@ --- name: pr -description: Create a PR from an already verified, committed, and pushed branch. Ready PRs return to the planner for delegated fixup. +description: Create a PR from a committed, post-commit-verified, and pushed branch. Ready PRs return to the planner for delegated fixup. --- # PR ## Planner Entry -The user-started primary session delegates -verification, commit, push, and PR creation as separate bounded implementer -assignments, then coordinates `/pr-fixup` unless draft mode was requested. It -does not run repository-host commands directly. +The planner may create a routine PR directly after commit, post-commit +verification, and push. It delegates only substantial delivery work; ready PR monitoring remains +with `pr-poller`/`pr-fixup` when long-running. An explicitly assigned PR worker creates the PR only after receiving the verified, committed, and pushed branch state. It does not spawn other workers. @@ -27,7 +26,8 @@ verified, committed, and pushed branch state. It does not spawn other workers. ## Available skills -- **`/commit`** — Planner-side prerequisite for verified, committed changes. +- **`/commit`** — Creates the artifact and hook receipt before verification. +- **`/verify`** — Mandatory post-commit gate before push and PR creation. - **`/pr-fixup`** — Wait for CI checks and CodeRabbit, Greptile, Claude, OpenCode, and cubic review feedback, fix any failures or valid comments, and push. ## Options @@ -42,7 +42,7 @@ Do not create, update, or delete Kandev subtasks for this workflow unless the us explicitly requests task tracking. 1. **Uncommitted changes:** If there are dirty or staged changes, stop and tell - the planner that verification and commit assignments are required first. + the planner that commit and post-commit verification are required first. 2. **Branch:** If on `main` or `master`, stop and tell the planner that a bounded branch-preparation assignment is required. Otherwise use the current diff --git a/.agents/skills/push/SKILL.md b/.agents/skills/push/SKILL.md index 67513c0698..e396b2f083 100644 --- a/.agents/skills/push/SKILL.md +++ b/.agents/skills/push/SKILL.md @@ -7,17 +7,17 @@ description: Push an already verified and committed branch. With --fixup, return ## Planner Entry -The user-started primary session delegates any -required verification and commit first, then assigns the push to an -`implementer` worker. With `--fixup`, the planner coordinates `/pr-fixup` -workers after the push. It does not run Git or GitHub commands directly. +The planner may push a routine verified commit directly. With `--fixup`, it +keeps long monitoring on delegated `pr-poller`; delegate delivery only when it +has a material isolation or coordination benefit. An explicitly assigned push worker pushes only the already verified and committed branch and does not spawn other workers. ## Available skills -- **`/commit`** — Planner-side prerequisite for verified, committed changes. +- **`/commit`** — Creates the artifact and hook receipt before verification. +- **`/verify`** — Mandatory post-commit gate for the exact current `HEAD`. - **`/pr-fixup`** — Wait for CI checks and CodeRabbit, Greptile, Claude, OpenCode, and cubic review feedback, fix any failures or valid comments, and push again. ## Options @@ -35,11 +35,15 @@ Push the already committed branch to its remote. **Create a todo/task for each step below and mark them as completed as you go.** 1. **Uncommitted changes:** If there are dirty or staged changes, stop and tell - the planner that verification and commit assignments are required first. + the planner that a new commit and verification run are required first. -2. **Safety check:** Verify the current branch is NOT `main` or `master`. If it is, stop and ask the user — direct pushes to the default branch should go through a PR. +2. **Verification evidence:** Require a successful post-commit `verify` result + whose reported `HEAD` exactly equals current `HEAD`. If the checkout or + commit changed afterward, stop and require fresh verification. -3. **Push** the current branch: +3. **Safety check:** Verify the current branch is NOT `main` or `master`. If it is, stop and ask the user — direct pushes to the default branch should go through a PR. + +4. **Push** the current branch: ```bash git push ``` @@ -53,7 +57,7 @@ Push the already committed branch to its remote. example `git push git@github.com:/.git `, or tell the user the token needs `workflow` scope. -4. **Report** the pushed commit hash and branch. +5. **Report** the pushed commit hash and branch. -5. **If `--fixup`:** Return the pushed branch state to the planner so it can +6. **If `--fixup`:** Return the pushed branch state to the planner so it can coordinate `/pr-fixup`. Do not poll, fix, verify, or spawn another worker. diff --git a/.agents/skills/simplify/SKILL.md b/.agents/skills/simplify/SKILL.md index 2b2e181b05..7973facde3 100644 --- a/.agents/skills/simplify/SKILL.md +++ b/.agents/skills/simplify/SKILL.md @@ -9,17 +9,16 @@ Post-implementation simplification pass. Review recently changed code and active ## Planner Entry -The user-started primary session delegates this -procedure to the registered `simplify` worker and assigns verification to the -`verify` worker afterward. It does not edit or verify the code directly. An -explicitly assigned simplify worker continues below and does not spawn workers. +The planner may simplify small localized changes directly; delegate larger or +cross-component work. Code/test/config changes still require final Spark +`verify`. A simplify worker owns one packet and does not spawn workers. The best code is code you don't have to write. The second best is code anyone can read. ## Available skills and subagents -- **`verify` worker** — The planner runs this after simplification to ensure all - tests, lints, and typechecks still pass. +- **`verify` worker** — After simplification is accepted and committed through + hooks, the planner runs this post-commit gate before push. ## Steps @@ -62,7 +61,7 @@ Report any verification concerns or focused checks the planner should delegate. ### 3. Verify -Report the required full verification and any focused concerns to the planner; +Report the required final change-aware verification and any focused concerns to the planner; do not run verification yourself. The planner delegates it to the `verify` worker. If anything breaks, the simplification changed behavior and the planner assigns the correction to a worker. diff --git a/.agents/skills/spec-driven-development/SKILL.md b/.agents/skills/spec-driven-development/SKILL.md index 5350b41f11..95966dc250 100644 --- a/.agents/skills/spec-driven-development/SKILL.md +++ b/.agents/skills/spec-driven-development/SKILL.md @@ -1,6 +1,6 @@ --- name: spec-driven-development -description: "Single planner entrypoint for Kandev spec-driven implementation. Use when developing a feature or behavior-changing fix through the full flow: clarify intent, create/update specs, create a plan, split independent verifiable tasks, delegate every execution step to workers, verify, and report progress." +description: "Single planner entrypoint for Kandev spec-driven implementation. Use when developing a feature or behavior-changing fix through the full flow: clarify intent, create/update specs, plan substantial delegated implementation, verify, and report progress." --- # Spec-Driven Development @@ -9,15 +9,19 @@ Use this as the default development workflow for non-trivial features and behavi ## Planner And Workers -The user-started primary session owns planning, -delegation, progress, approvals, and user communication. It does not implement, -test, integrate, verify, or ship code. Workers do not spawn other workers. +The user-started primary session owns architecture, planning, integration +judgment, delegation, and user +communication. Large feature-plan implementation stays worker-driven; small +scoped work may be direct under `/planner-orchestration`. Workers do not spawn. - **`architect`** - optional frontier-model design reviewer for unusually risky architecture; it is not the default planner. - **`implementer`** - implements one assigned task with TDD in the current worktree or an assigned git worktree. - **`test-engineer`** - designs or adds focused tests when coverage is unclear, a bug needs a Prove-It regression, or a task is test-heavy. -- **`qa`** - validates integrated waves against the spec/plan, checks wiring, tests edge cases, and reports readiness. -- **`security-auditor`** - reviews security-sensitive changes such as auth, workspace isolation, filesystem/process execution, integrations, webhooks, secrets, or agent/tool permissions. +- **`qa`** - exceptional independent validation for unusually complex multi-component behavior or an important boundary lacking faithful tests. +- **`code-review`** - exceptional local frontier review; qualifying current-head PR AI evidence is the default semantic path for PR delivery. +- **`security-auditor`** - exceptional audit for high-impact new/changed security boundaries or concrete security concerns. +- **`spark-explorer`** - Codex-only opt-in read-only specialist for bounded call-path tracing and evidence gathering; never use it for architecture, security, final review, implementation, or edits. +- **`spark-implementer`** - Codex-only opt-in specialist for explicit localized low-risk UI or code edits; use the normal implementer for work outside its stop conditions. If a required worker cannot be launched, stop and report the unavailable role. Never execute that worker's phase in the planner session. @@ -25,7 +29,7 @@ Never execute that worker's phase in the planner session. ## Core Flow ```text -Intent -> Planner artifacts -> User approval -> Workers by wave -> QA -> Verify -> Report +Intent -> Planner artifacts -> User approval -> Workers by wave -> Risk-routed evidence -> Commit -> Verify -> Report ``` Do not skip from intent to code unless the user explicitly asks to bypass the process. @@ -125,7 +129,7 @@ Each task needs: - **Verification:** exact targeted command(s). - **Files:** specific paths, not broad directories. - **Inputs:** spec section, plan section, relevant patterns, and dependencies. -- **Output contract:** summary of changes, tests run, files touched, blockers, and follow-up risks. +- **Output contract:** compact handoff capsule: intent/acceptance; base/head SHA when applicable; changed files and entry points; named spec/ADR sections; risk tags; exact targeted commands/results; uncertainties. - **Dependencies:** task IDs that must complete first, or `None`. Independence test: @@ -161,8 +165,8 @@ Parallelize only when safe: If the active harness's native subagent tools and git worktrees are available, the planner may request one worktree per independent task. Worktree creation, -branch operations, and integration are bounded implementer assignments; the -planner does not run Git commands itself. +branch operations, and integration are delegated when their isolation benefit +is material; the planner may handle a small routine Git step directly. The assigned setup worker may use: @@ -190,7 +194,12 @@ For each task: `plan.md` unless the work packet explicitly owns that shared file and the update is serialized outside parallel execution. -Assign every independent task to an `implementer` worker. Launch implementers in parallel only for tasks in the same wave that do not share mutable files. Use this prompt shape: +Assign every independent task to the normal `implementer` worker by default. +Codex may use `spark-implementer` only for an explicit, localized low-risk UI +or code edit when none of its documented stop conditions apply; if there is +doubt, use the normal `implementer`. Keep the same file-ownership, wave, TDD, +and output rules for either role. Launch workers in parallel only for tasks in +the same wave that do not share mutable files. Use this prompt shape: ```text Task: @@ -208,28 +217,51 @@ Constraints: - Update only the assigned task file; do not edit `plan.md` without explicit, serialized shared-plan ownership. Output: -- Summary, files changed, tests run, blockers, risks, and task file status update. +- Compact handoff capsule (intent/acceptance; base/head SHA when applicable; + changed files/entry points; named spec/ADR sections; risk tags; exact + targeted commands/results; uncertainties) and task file status update. ``` -The planner coordinates waves and keeps progress state. Conflict resolution, -branch integration, and follow-up fixes are new bounded `implementer` -assignments. The planner never handles them inline. +The planner coordinates waves and keeps progress state. Delegate substantial +conflict resolution, integration, and follow-up fixes; handle only small scoped +ones directly under `/planner-orchestration`. ## Phase 7: Integrate And Verify +Per-task versus per-wave checks are distinct: + +- Each assigned `implementer` or `spark-implementer` owns and runs its task's + targeted verification. The planner checks the reported evidence; it does not + duplicate those tests after every wave. +- Derive risk tags from worker reports plus planner inspection and apply the + `/planner-orchestration` evidence contract. Ordinary integration with + faithful tests does not automatically require `qa`. + After each wave: -- Delegate targeted tests for changed backend packages or frontend modules. - Delegate conflict resolution and rerun affected tests through an implementer. - Update the plan if the task files or wave graph changed. At the end: - Run the `test-engineer` worker when coverage is disputed, missing, or hard to place at the right test level. -- Run the `qa` worker for integration and behavior verification. -- Run the `security-auditor` subagent for security-sensitive changes before declaring readiness. - Run the `simplify` worker if implementation grew speculative abstractions. -- Run the `verify` worker for full format, typecheck, tests, and lint. +- For PR delivery, defer routine semantic review until one qualifying + current-head PR AI review; use local `code-review` only for the central + exceptional routes. +- Run `qa` and `security-auditor` only for their central exceptional routes. +- Commit through active hooks, then run mandatory final change-aware `verify` + before push; pass the hook receipt and use full mode when triggered. - Use `/record` for ADR/spec updates if implementation discovered a durable decision or behavior change. +When PR delivery is in scope, PR AI review may be deferred, but do not claim +readiness until qualifying current-head semantic-review evidence exists. + +Route every finding to a new bounded implementer packet. For a small, +scope-preserving PR remediation, use the finding, focused regression, final +Spark `verify`, and fresh qualifying exact-head PR AI review; do not relaunch +local gates unless the fix meets a central exceptional route. Reuse the same +native thread when role, change, and file scope remain materially the same; +otherwise launch a fresh worker for independent judgment or stale/noisy context. + ## Stop Conditions Stop and ask the user when: @@ -246,4 +278,5 @@ Report: - Task waves completed and any tasks left. - Subagent/worktree branches used, if any. - Tests and verification commands run. +- Semantic-review evidence, risk tags, and QA decision/reason. - Known pending checks or risks. diff --git a/.agents/skills/tdd/SKILL.md b/.agents/skills/tdd/SKILL.md index 602808e293..99fd2e9622 100644 --- a/.agents/skills/tdd/SKILL.md +++ b/.agents/skills/tdd/SKILL.md @@ -7,10 +7,9 @@ description: Implement changes using Test-Driven Development (Red-Green-Refactor ## Execution Context -The user-started primary session delegates TDD -work to an `implementer` or `test-engineer` worker and does not continue below. -An explicitly assigned worker follows this procedure and does not spawn other -workers. +The planner may apply TDD directly for small scoped work; delegate substantial, +cross-component, or independently test-heavy work. A worker follows this +procedure for its one packet and does not spawn workers. Implement code changes using strict Red-Green-Refactor. Iron law: **no production code without a failing test first.** @@ -19,7 +18,8 @@ Wrote code before a test? Delete it. Start over from a failing test. ## Available skills and subagents - **`/e2e`** — Follow this procedure when the assigned packet explicitly owns Playwright E2E coverage. -- **`/verify`** — The planner launches this as a separate assignment after the worker's targeted checks pass. +- **`/verify`** — After targeted checks pass, the planner commits through hooks + and launches this as a separate post-commit assignment before push. ## When to use @@ -96,7 +96,8 @@ Return to step 1 for the next behavior or edge case. Continue until the feature ### 5. Final verification Run the targeted tests named in the work packet and report their results. The -planner launches a separate `verify` assignment for the monorepo pipeline. +planner commits the accepted result, then launches a separate hook-aware +`verify` assignment before push. ## Testing anti-patterns diff --git a/.agents/skills/using-agent-skills/SKILL.md b/.agents/skills/using-agent-skills/SKILL.md index edc8993207..466a030df2 100644 --- a/.agents/skills/using-agent-skills/SKILL.md +++ b/.agents/skills/using-agent-skills/SKILL.md @@ -10,12 +10,10 @@ Use this as the routing map for Kandev's local skills. Prefer the repo's existin ## Session Role The root `AGENTS.md` (also supplied to Claude through `CLAUDE.md`) is the -authoritative always-on planner/worker role boundary. The user-started primary -session plans and delegates through the active harness's native subagent tool; -registered custom agents execute bounded worker assignments. Execution skills -below describe what the assigned worker does, not work the planner performs -directly. Never use Kandev MCP task/session APIs as worker delegation or as a -fallback for unavailable native subagents. +authoritative role boundary. The planner is the default architect and may apply +execution skills directly to small scoped work; workers provide bounded +isolation or independent evidence when their ROI is positive. Never use Kandev +MCP task/session APIs as worker delegation or as a fallback. Load `/planner-orchestration` only when its detailed work-packet, model-tier, or completion rules are needed. Execution skills do not each need to load it. @@ -36,11 +34,11 @@ Task arrives |-- Seed isolated product demo data? -------> /product-demo-seeding |-- Record landing/product media? ----------> /product-demo-seeding -> /product-video-capture (always in that order) |-- Frontend/UI change? --------------------> /mobile-parity plus /e2e as needed -|-- Security-sensitive change? -------------> security-auditor subagent plus /code-review +|-- High-impact security boundary/concern? -> security-auditor per /planner-orchestration |-- Test strategy or coverage gaps? --------> test-engineer subagent plus /tdd or /e2e |-- Add debug logs? ------------------------> /debug |-- Add Jira/Linear-style integration? -----> /add-integration -|-- Verify implemented behavior? -----------> /qa then /verify +|-- Verify implemented behavior? -----------> PR-first review, exceptional /qa, mandatory /verify |-- Simplify recent code? ------------------> /simplify |-- Review code? ---------------------------> /code-review |-- Improve skills/agents/commands? --------> /harness-improvement @@ -59,7 +57,8 @@ Task arrives 5. Surface assumptions before building on them. If requirements, specs, and code disagree, stop and name the conflict. 6. Keep scope tight. Do not refactor adjacent systems or add "useful" features that are not in the request/spec. 7. Verify with evidence: targeted tests, full `/verify` when needed, browser/E2E proof for user-facing flows. -8. The planner never uses a direct execution fallback when a worker is unavailable. +8. Keep small safe work local when context reload and coordination cost exceed + delegation value; preserve worker packet/no-subagent rules when delegating. 9. Product media always invokes `/product-demo-seeding` before `/product-video-capture`, even when a prior seed or capture exists. Re-prove current `origin/main`, disposable runtime/data, and teardown; never capture a developer instance or database. ## Upstream Name Mapping diff --git a/.agents/skills/verify/SKILL.md b/.agents/skills/verify/SKILL.md index 671bf98c6d..32bf1c2d4e 100644 --- a/.agents/skills/verify/SKILL.md +++ b/.agents/skills/verify/SKILL.md @@ -1,20 +1,22 @@ --- name: verify -description: Run format, typecheck, test, and lint across the monorepo. Use after implementing changes. +description: Verify the committed artifact before push, reusing proven pre-commit hook coverage and escalating to full checks when impact requires it. --- # Verify -In the user-started primary session, delegate the -full verification pipeline to the registered `verify` worker. Do not substitute +In the user-started primary session, delegate verification to the registered +Spark `verify` worker after commit and before push, with `mode=changed` by +default. Supply the `/commit` hook receipt and last successfully verified SHA +when available. Do not substitute a generic agent: it may lack the required GitHub network access or shared-worktree write permissions. If the worker cannot be launched or access the required resources, stop and report that verification is blocked. An explicitly assigned `verify` worker follows the worker procedure below, reports verification failures without fixing source or test logic, and does not -spawn workers. The planner assigns failures to an `implementer`, then launches -a fresh `verify` assignment. +spawn workers. The planner handles a small remediation directly or delegates a +larger one, then launches a fresh `verify` assignment. ## What to do @@ -24,10 +26,10 @@ Invoke the `verify` worker in a single call. Wait for it to complete and surface - If verify fails: create a bounded remediation assignment from its report and do not proceed with downstream actions that depend on green verification. - If verify reports that required sandbox capabilities could not be authorized, - stop before commit, push, or PR creation and surface its required user action. + stop before push or PR delivery and surface its required user action. On Codex, tell the user exactly: "Switch the mode selector to - `Agent (full access)`, then retry verification." Explain that PR creation is - waiting on mandatory verification; do not imply that Codex or GitHub cannot + `Agent (full access)`, then retry verification." Explain that push and PR + delivery are waiting on mandatory verification; do not imply that Codex or GitHub cannot create PRs, ask whether to proceed unverified, or launch downstream workers. Do not run verification commands in the planner session. The worker's prompt @@ -35,8 +37,21 @@ contains the full procedure in `.agents/agents/verify.md`. ## Worker Procedure -The assigned `verify` worker runs the full pipeline from the repository root and -reports the exact commands and results: +Resolve the PR base and verification scope base, then collect +`scope-base...HEAD`, staged, unstaged, and untracked paths. The supplied last +verified SHA may be the scope base only when it is an ancestor of `HEAD`; +otherwise use the PR base. Report PR base/head, scope base, paths/categories, +hook-receipt eligibility and omissions, exact commands, and coverage limits. +If base/diff is unavailable or impact is ambiguous, use `mode=full`; use full +mode for explicit requests, releases, shared build or toolchain changes, and +unusually broad work. PR CI is the authoritative full matrix. Read +[impact-matrix.md](references/impact-matrix.md) and, when a receipt is +supplied, [hook-evidence.md](references/hook-evidence.md) before commands. A +scoped pass is `changed-scope PASS`, never `full PASS`. + +In `mode=full`, run the pipeline below and ignore hook omissions. In +`mode=changed`, run only uncovered matrix commands for impacted categories; do +not run unrelated suites or repeat eligible hook-covered formatting/lint. ```bash # Fresh worktrees share .git/ but not apps/node_modules. @@ -113,7 +128,7 @@ still fails. If that escalation is unavailable, denied, cancelled, or interrupted, stop and return a blocked verification report with a **Required user action** section. -State that mandatory verification must pass before commit and PR creation can +State that mandatory verification must pass before push and PR delivery can continue. On Codex, the action must say exactly: "Switch the mode selector to `Agent (full access)`, then retry verification." On other runtimes, tell the user to enable the runtime's full filesystem, network, or loopback access as @@ -135,11 +150,13 @@ those require a bounded implementer assignment before verification continues. For source, test, type, or lint failures, stop after capturing targeted failure evidence. Report the command, quiet-log path and relevant lines, likely owned files, and a concise remediation recommendation. Do not edit logic or rerun the -whole pipeline until the planner has assigned and integrated a fix. +selected checks until the planner has integrated a fix. -If `make fmt` changes files, review and report the formatter diff, then continue -with the remaining commands. If a later command fails, capture targeted evidence -and stop for planner-assigned remediation. +If formatting changes files after commit, review and report the formatter diff, +invalidate the hook receipt and verified-commit state, then continue only to +collect useful evidence. The planner must commit the formatter result and +launch fresh verification before push. If a later command fails, capture +targeted evidence and stop for planner-assigned remediation. `make test` includes backend, web, CLI, and `test-scripts`; do not silently skip `test-scripts` or its desktop smoke coverage while reporting full verification diff --git a/.agents/skills/verify/references/hook-evidence.md b/.agents/skills/verify/references/hook-evidence.md new file mode 100644 index 0000000000..7e6f1ce77a --- /dev/null +++ b/.agents/skills/verify/references/hook-evidence.md @@ -0,0 +1,54 @@ +# Hook Evidence + +Use a commit hook receipt only to remove exact duplicate work from +`mode=changed`. The planner supplies the receipt from `/commit` plus the last +successfully verified SHA. Do not infer hook success from commit history. +When the receipt is eligible, every covered check below must be omitted; do not +rerun it for reassurance. + +## Eligibility + +All conditions are required: + +- authoritative PR CI exists for delivery; +- mode remains `changed`; +- `receipt.commit_sha` equals current `HEAD`; +- `receipt.parent_sha` equals both `HEAD^` and the supplied last verified SHA + (or the PR base for the first verified commit); +- pre-commit and commit-msg hooks were active; +- `receipt.bypass` is false and `receipt.commit_result` is `pass`; +- the receipt names each hook as `passed` or `skipped`; +- the worktree, index, and untracked set are clean; and +- `.pre-commit-config.yaml`, hook wiring, root build/toolchain files, or hook + implementation did not change in the receipt range. + +If any condition is unknown or false, run the normal impact-matrix commands. +Full mode and delivery without PR CI never use hook omissions. + +## Covered Checks + +Omit a covered check when the named hook reported `passed` for at least one +matching path in the receipt range. `verify` must not execute that duplicate +command after declaring the receipt eligible. + +| Successful hook | Changed-scope work it replaces | +| --- | --- | +| `harness-lint` | Targeted harness lint for committed harness/instruction files | +| `gofmt` | Formatting of committed `apps/backend/**/*.go` files | +| `go-lint` | Changed-code backend lint for the receipt range | +| `prettier-format` | Formatting of committed matching web/CLI/packages files | +| `web-lint` | Changed-code web lint for committed matching JS/TS files | +| `commitlint` | Commit-message validation only; `verify` has no equivalent | + +`skipped` is not `passed` and grants no coverage. + +## Never Covered + +Hooks do not replace `git diff --check`, TOML parsing, generated metadata, +typechecking, tests, builds, public-doc validation, action pinning, script +syntax/tests, CLI tests, desktop/Rust checks, or any full-mode command. Run +every applicable uncovered command from the impact matrix. + +Report the receipt decision, every omitted check and covering hook, and every +remaining command. A pass using eligible omissions is still +`changed-scope PASS`; it is not a full pass. diff --git a/.agents/skills/verify/references/impact-matrix.md b/.agents/skills/verify/references/impact-matrix.md new file mode 100644 index 0000000000..8f89918364 --- /dev/null +++ b/.agents/skills/verify/references/impact-matrix.md @@ -0,0 +1,34 @@ +# Verify Impact Matrix + +Collect `base...HEAD`, staged, unstaged, and untracked paths. Deduplicate the +union, then run every matching row. Prefer package/suite targets over +individual test names so changed dependents remain covered. + +When the planner supplies a last verified SHA and a `/commit` hook receipt, +read [hook-evidence.md](hook-evidence.md). Eligible hook evidence removes the +duplicate formatting/lint portions below; do not rerun them, and run every +uncovered command. + +| Paths | Changed-mode commands | +| --- | --- | +| `AGENTS.md`, `CLAUDE.md`, `.agents/**`, `.claude/**`, `.codex/**`, `.cursor/**`, `.opencode/**`, ADRs/plans/specs | `git diff --check` and targeted `.github/scripts/lint-harness-files.py` inputs; parse changed TOML | +| `docs/public/**` | `node --test scripts/validate-public-docs.test.mjs` plus applicable harness lint | +| `.github/workflows/**` | `python3 .github/scripts/lint-action-pinning_test.py` plus applicable harness lint | +| `scripts/**`, `.github/scripts/**` | sibling syntax/test when obvious; otherwise `make test-scripts` | +| `apps/backend/**` | `make fmt-backend`, `make test-backend`, `make lint-backend` | +| `apps/web/**` | generate web metadata, `make fmt-web`, `make typecheck-web`, `make test-web`, `make lint-web` | +| `apps/cli/**` | workspace format, CLI TypeScript check/build, `make test-cli` | +| `apps/desktop/**` excluding `src-tauri` | desktop TypeScript check and the directly affected desktop smoke/test | +| `apps/desktop/src-tauri/**` | matching Rust toolchain, `cargo fmt --check`, `cargo check`, and scoped `cargo test` | +| `apps/packages/**` or shared TypeScript config | web metadata, workspace format/typecheck, web and CLI tests, web lint, desktop typecheck | + +Use `mode=full` when the base/diff cannot be established; a changed path has no +safe row; the user requests it; delivery has no PR CI; or changes touch root +build/toolchain/dependency lockfiles, Makefiles, profiles, generated contracts, +release tooling, migrations/shared schemas, or unusually broad plan +implementation. Multiple known rows are not automatically ambiguous: run their +union when ownership and dependents are clear. + +Do not run E2E unless acceptance or changed behavior requires it; targeted E2E +is separate evidence. A changed-scope pass permits commit/push, while PR CI +supplies the authoritative full matrix before readiness. diff --git a/.claude/commands/pr-fixup.md b/.claude/commands/pr-fixup.md index d230989cdc..e5b877eb5a 100644 --- a/.claude/commands/pr-fixup.md +++ b/.claude/commands/pr-fixup.md @@ -9,8 +9,9 @@ effort: high Rely on the root `AGENTS.md`/`CLAUDE.md` planner/worker contract and coordinate `.agents/skills/pr-fixup/SKILL.md` as the primary planner. -Delegate PR state collection to `pr-poller`, remediation to `implementer`, full -checks to `verify`, and commit/push to a bounded delivery assignment. Review +Delegate PR state collection to `pr-poller` and broad remediation to +`implementer`. Commit focused fixes through active hooks, then delegate +post-commit checks to `verify` before push. Review each compact result and launch another bounded assignment when needed. Do not run GitHub, edit, test, commit, or push commands in this primary session. If a required worker cannot be launched, stop and report the blocked phase. diff --git a/.codex/agents/architect.toml b/.codex/agents/architect.toml index 2c823c565e..5d1f357b8b 100644 --- a/.codex/agents/architect.toml +++ b/.codex/agents/architect.toml @@ -1,10 +1,10 @@ name = "architect" -description = "Provide a frontier-model second opinion on unusually risky Kandev architecture and planning; the primary planner owns normal specs and plans." +description = "User-requested independent frontier second opinion; planner owns normal architecture, specs, and plans." model = "gpt-5.6-sol" model_reasoning_effort = "high" sandbox_mode = "read-only" developer_instructions = """ -Review one bounded architecture question from the primary planner. Read the named spec, plan, ADR, source, tests, constraints, and alternatives. Check ownership boundaries, contracts, persistence, permissions, failure modes, concurrency, migration risk, and verification strategy as relevant. +Invoke only when the user explicitly requests an independent architecture second opinion. Review one bounded architecture question from the primary planner. Read the named spec, plan, ADR, source, tests, constraints, and alternatives. Check ownership boundaries, contracts, persistence, permissions, failure modes, concurrency, migration risk, and verification strategy as relevant. Return a recommendation, risks and mitigations, rejected alternatives, required changes to the planner's artifacts, and open decisions. Do not edit files. Do not spawn subagents. """ diff --git a/.codex/agents/code-review.toml b/.codex/agents/code-review.toml index b5502b367a..a7a847591d 100644 --- a/.codex/agents/code-review.toml +++ b/.codex/agents/code-review.toml @@ -1,5 +1,5 @@ name = "code-review" -description = "Review changed Kandev code for correctness, security, architecture, scope, complexity, and missing tests after implementation or before PRs." +description = "Frontier-review Kandev changes with architecture, cross-cutting, high-risk, or stale/incomplete external-review risk." model = "gpt-5.6-sol" model_reasoning_effort = "high" sandbox_mode = "read-only" diff --git a/.codex/agents/pr-poller.toml b/.codex/agents/pr-poller.toml index 0d01a24ab3..5117b7a0f7 100644 --- a/.codex/agents/pr-poller.toml +++ b/.codex/agents/pr-poller.toml @@ -8,13 +8,26 @@ Pure polling role. Do not read source, edit files, push, reply to comments, reso Before the first GitHub-dependent helper or gh call, check the inherited runtime policy. If network access is already granted, including in a full-access or approval-policy-never session, invoke exec_command normally without a sandbox_permissions override. When network access requires escalation and the approval policy supports prompting, invoke exec_command with sandbox_permissions set to "require_escalated" and a concise justification asking the user to approve GitHub access. Never pass require_escalated when the approval policy is never; rejection of an incompatible override is not an approval denial. Request approval proactively when it is required and available rather than waiting for an unapproved command to fail. If the network escalation or approval request is denied, cancelled, or interrupted, make no further tool call, emit unknown for every unobserved GitHub-derived field, and return the normal report with this exact recommendation: GitHub access requires approval; planner must surface the approval gate to the user and must not relaunch polling. This is a terminal user gate, not a fetch failure. Only retry a transient DNS, API, or transport error after access was granted and the GitHub command actually ran. -Use raw scripts/pr-state <PR> to derive bot states and counts from its reviews and issue_comments arrays; summary mode omits those arrays. Prefer scripts/pr-state --summary <PR> and scripts/pr-resolve list <PR> for compact CI and thread state over raw gh parsing. Use one-shot checks or bounded commands rather than long inline shell loops. +The planner may supply selected_reviewer=<GitHub login>. Selected evidence qualifies when checks_head_sha, opening_head_sha, and closing_head_sha are known and identical, checks_snapshot_complete is true, the reviewer has an exact_current_head record with commit_id equal to evidence_head_sha, and blocker/thread/actionable-comment gates are zero. Only the dedicated OpenCode App `${OPENCODE_REVIEW_APP_SLUG}[bot]` additionally requires emitted trusted_producer=true; use raw scripts/pr-state --trusted-reviewer <login> <PR> for that path. Other explicitly selected reviewers retain generic exact-head qualification regardless of trusted_producer. Displayed check names never authenticate the App path. The App shortcut is routine same-repository work only; forks, security, architecture, missing configuration, API-read failure, or a latest non-success workflow/job require local frontier review. + +After CI, selected-reviewer qualified or blocked evidence may end polling. For +each reviewer, report the latest exact-current-head record; separately aggregate +all exact-head blocked or conflicting records, which must remain zero. Do not +weaken same-head non-clean blocking semantics. Use the exact marker-delimited output contract from .agents/agents/pr-poller.md: === pr-poller report === through === end ===, with pr/branch, head_sha, mergeable, merge_state_status, local_unmerged_entries, ci_failed, ci_passed, ci_pending, each bots.* row, unresolved_review_threads, -issue_comments_from_bots, claude_summary, and recommendation. Omit no field +checks_evidence, actionable_issue_comments_from_bots, claude_summary, review_evidence, and recommendation. The +parseable review_evidence line is: reviewer=<login|none> +qualification=<qualified|blocked|unqualified|unknown> +reviewed_head_sha=<SHA|unknown> review_state=<state|none|unknown> +eligibility=<eligible|blocked|ineligible|unknown> +verdict=<approved|clean|blocker|dismissed|unknown|none> +trusted_producer=<true|false|unknown> +active_changes_requested=<N|unknown> +blocked_exact_current_head_reviews=<N|unknown>. Omit no field except the documented empty ci_failed line. Report only observed values. Use gh pr view for PR fields and git ls-files -u for local index state. If data cannot be fetched, use unknown and explain the fetch failure in recommendation. Return only the report block and exit. diff --git a/.codex/agents/qa.toml b/.codex/agents/qa.toml index 2161311fd4..02a1ccce68 100644 --- a/.codex/agents/qa.toml +++ b/.codex/agents/qa.toml @@ -1,5 +1,5 @@ name = "qa" -description = "Verify integrated Kandev work after implementation by tracing wiring, testing happy paths, breaking edge cases, and checking coverage/readiness." +description = "Independently verify Kandev changes with integration, public-contract, persistence, concurrency, recovery, cross-component, or missing faithful behavior evidence." model = "gpt-5.6-terra" model_reasoning_effort = "medium" sandbox_mode = "read-only" diff --git a/.codex/agents/simplify.toml b/.codex/agents/simplify.toml index 145e57da19..7f84040866 100644 --- a/.codex/agents/simplify.toml +++ b/.codex/agents/simplify.toml @@ -9,5 +9,5 @@ Review recently changed files and simplify while preserving behavior. Look for one-off abstractions, wrapper types, single-implementation interfaces, unused parameters, speculative flags, reserved-for-future scaffolding, deep nesting, clever dense code, noisy comments, redundant annotations, and leftover debug logging. Verify after meaningful simplifications with the relevant focused checks. If a simplification changes behavior, revert it. Report files modified, what was removed/inlined/simplified, and verification run. -Do not spawn subagents. Report required full verification to the planner. +Do not spawn subagents. Report required change-aware verification to the planner. """ diff --git a/.codex/agents/spark-explorer.toml b/.codex/agents/spark-explorer.toml new file mode 100644 index 0000000000..27a2adbcae --- /dev/null +++ b/.codex/agents/spark-explorer.toml @@ -0,0 +1,12 @@ +name = "spark-explorer" +description = "Codex-only read-only GPT-5.3-Codex-Spark specialist for bounded exploration, call-path tracing, and evidence gathering." +model = "gpt-5.3-codex-spark" +model_reasoning_effort = "medium" +sandbox_mode = "read-only" +developer_instructions = """ +Execute exactly one bounded exploration assignment. Require a concrete question, relevant paths or symptom, file scope, and expected evidence before starting. + +Read scoped AGENTS.md and only the source, tests, history, or runtime evidence needed to answer the question. Trace call paths and report evidence with file references, likely root cause or decision-relevant facts, uncertainties, and recommended next owner. + +Do not make edits or run mutating commands. Do not perform architecture design, security auditing, final review, or implementation. Stop and return to the planner when the question needs any of those roles, the scope is ambiguous, or evidence is insufficient. Do not spawn subagents. +""" diff --git a/.codex/agents/spark-implementer.toml b/.codex/agents/spark-implementer.toml new file mode 100644 index 0000000000..a6020d8a99 --- /dev/null +++ b/.codex/agents/spark-implementer.toml @@ -0,0 +1,12 @@ +name = "spark-implementer" +description = "Codex-only GPT-5.3-Codex-Spark specialist for explicit targeted UI changes or localized low-risk edits with exact verification." +model = "gpt-5.3-codex-spark" +model_reasoning_effort = "medium" +sandbox_mode = "workspace-write" +developer_instructions = """ +Execute exactly one targeted, low-risk assignment. Require title, goal, explicit acceptance criteria, exact verification commands, file scope, dependency status, and relevant spec/plan excerpts or standalone bug/reproduction inputs before starting. + +Use TDD where code changes need coverage: write or update the smallest failing test first, implement narrowly, then run the assigned tests and verification. For UI work, apply E2E and mobile-parity guidance. Report behavior implemented, files changed, commands and results, blockers, risks, and divergence. + +Stop and return to the planner without editing when ambiguity, cross-subsystem work, auth, security, workspace isolation, concurrency, persistence or migrations, process or filesystem execution, public contracts, or architectural changes are involved. Do not broaden scope, refactor adjacent systems, or edit files owned by parallel tasks. Do not spawn subagents. +""" diff --git a/.codex/agents/verify.toml b/.codex/agents/verify.toml index 07f4ea362e..2a5f9f33a6 100644 --- a/.codex/agents/verify.toml +++ b/.codex/agents/verify.toml @@ -1,15 +1,15 @@ name = "verify" -description = "Run Kandev format, typecheck, tests, and lint before commit, then report failures without fixing source or test logic." -model = "gpt-5.6-luna" -model_reasoning_effort = "low" +description = "Verify committed changes before push, reusing proven hook coverage and escalating only when impact requires it." +model = "gpt-5.3-codex-spark" +model_reasoning_effort = "medium" sandbox_mode = "workspace-write" developer_instructions = """ -Run the monorepo verification pipeline and report issues found. +Run after commit and before push. Default to mode=changed: resolve the PR base and supplied last verified SHA, collect scope-base...HEAD plus staged, unstaged, and untracked paths, and report mode, scope, commands, omissions, and limits. Validate any supplied /commit receipt with .agents/skills/verify/references/hook-evidence.md; never infer hook success. You MUST omit exact eligible hook-covered formatting/lint and MUST NOT rerun it for reassurance. Use full for ambiguous/broad impact, explicit request, releases/toolchain changes, or no PR CI. Read the impact matrix. PR CI is the authoritative full matrix; scoped success is changed-scope PASS. Install apps dependencies when missing. Resolve the current PR base with gh pr view --json baseRefName; fetch and report whether it is an ancestor of HEAD. Do not rebase, stash, resolve conflicts, or infer stacked-PR bases from Git upstream. -Generate web metadata, then run make fmt, make typecheck, make test, and make lint through scripts/run-quiet. Inspect only targeted log ranges. Full verification requires every test subtarget, including CLI, scripts, and desktop smoke coverage; run scoped Rust tests for Rust/Tauri changes after checking the required rust-version. +In changed mode, run only uncovered impact-matrix commands. Tests, typechecks, metadata, script/docs/workflow validators, TOML parsing, and desktop/Rust checks are never hook-covered. In full mode, ignore hook omissions: generate web metadata, then run make fmt, make typecheck, make test, and make lint through scripts/run-quiet. Inspect only targeted log ranges. Full verification requires every test subtarget, including CLI, scripts, and desktop smoke coverage; run scoped Rust tests for Rust/Tauri changes after checking the required rust-version. -Do not fix production or test logic. Retry environment-only failures with normal sandbox escalation and invocation-specific writable temp/Go/lint caches. If required filesystem, network, or loopback escalation is unavailable, denied, cancelled, or interrupted, stop and report verification as blocked. Explain that PR creation is waiting on mandatory verification and include this exact required user action: "Switch the mode selector to `Agent (full access)`, then retry verification." Do not offer to proceed unverified or imply that Codex or GitHub cannot create PRs. Recommend full access only after normal escalation could not authorize the required capability. For source failures, return the failed command, targeted log evidence, likely files, and a remediation recommendation for an implementer. Finish with a compact pass/fail report. +Do not fix production or test logic. A formatter edit after commit invalidates the receipt and requires a new commit plus verification before push. Retry environment-only failures with normal sandbox escalation and invocation-specific writable temp/Go/lint caches. If required filesystem, network, or loopback escalation is unavailable, denied, cancelled, or interrupted, stop and report verification as blocked. Explain that push and PR delivery are waiting on mandatory verification and include this exact required user action: "Switch the mode selector to `Agent (full access)`, then retry verification." Do not offer to proceed unverified or imply that Codex or GitHub cannot create PRs. Recommend full access only after normal escalation could not authorize the required capability. For source failures, return the failed command, targeted log evidence, likely files, and a remediation recommendation for an implementer. Finish with a compact pass/fail report. Do not spawn subagents. """ diff --git a/.cursor/agents/architect.md b/.cursor/agents/architect.md index 5a0ad311be..d659f0e4ae 100644 --- a/.cursor/agents/architect.md +++ b/.cursor/agents/architect.md @@ -1,6 +1,6 @@ --- name: architect -description: Provide a frontier-model second opinion on unusually risky Kandev architecture and planning. +description: User-requested independent frontier second opinion; planner owns normal architecture and plans. model: grok-4.5 readonly: true --- diff --git a/.cursor/agents/code-review.md b/.cursor/agents/code-review.md index c6e4b703d5..e18e69941e 100644 --- a/.cursor/agents/code-review.md +++ b/.cursor/agents/code-review.md @@ -1,6 +1,6 @@ --- name: code-review -description: Deeply review changed Kandev code for correctness, security, architecture, scope, and missing tests. +description: Frontier-review Kandev changes with architecture, cross-cutting, high-risk, or stale/incomplete external-review risk. model: grok-4.5 readonly: true --- diff --git a/.cursor/agents/pr-poller.md b/.cursor/agents/pr-poller.md index 6665b32db4..48e89c0bda 100644 --- a/.cursor/agents/pr-poller.md +++ b/.cursor/agents/pr-poller.md @@ -10,6 +10,21 @@ report including head SHA, GitHub mergeability/merge-state status, and local unmerged-index count. Do not inspect source, edit, push, reply, resolve threads, or fetch full logs. Do not spawn subagents. +The planner may provide `selected_reviewer=<GitHub login>`. Read raw +`scripts/pr-state --trusted-reviewer "$selected_reviewer" <PR>` when selection is set: review evidence is qualified +only when `checks_head_sha` plus opening/closing heads match and +`checks_snapshot_complete=true`, and an exact-current-head record has matching +`commit_id` to `evidence_head_sha`; an H1 check snapshot and H2 reviews are +unknown/nonqualifying and must retry or fall back. Timestamps never prove +head coverage. Only `eligibility=eligible` (approved with no blocker or explicit +`<!-- kandev-review: clean -->`) qualifies; for selected `${OPENCODE_REVIEW_APP_SLUG}[bot]`, +the emitted `trusted_producer=true` from `scripts/pr-state --trusted-reviewer "$selected_reviewer" <PR>`; only the dedicated OpenCode App requires this provenance, while other selected reviewers retain generic exact-head qualification. Generic marker reviews never qualify. Ambiguous, dismissed, pending, and +unknown reviews do not. Nonzero structured blockers, independently labeled Markdown `Blocker:`/`Blockers:` lines with descriptive values, or explicit blocked/changes-requested/action-required/must-fix evidence block; `No blocker(s): ...`, `Blocker(s): 0`, and `| Blocker | 0 |` do not. After CI completes, selected reviewer qualification or +observed blockers (active changes requested, any +`blocked_exact_current_head_review_count > 0`, `eligibility=blocked`, or +actionable unresolved threads) may end polling without waiting for unrelated +reviewers. Without selection, wait for the normal configured-reviewer terminal condition. + Before the first GitHub-dependent helper or `gh` call, request network access through Cursor's permission flow; do not run an unapproved probe first. If the request is denied, cancelled, or interrupted, make no further tool call, emit @@ -22,9 +37,16 @@ a fetch failure eligible for retry. Follow the exact output contract in `.agents/agents/pr-poller.md`: delimit the only output with `=== pr-poller report ===` and `=== end ===`, and include `pr`/`branch`, `head_sha`, `mergeable`, `merge_state_status`, -`local_unmerged_entries`, `ci_passed`, `ci_pending`, every bot row, -`unresolved_review_threads`, `issue_comments_from_bots`, `claude_summary`, and -`recommendation`. Emit a non-empty `ci_failed` list for observed failures, +`local_unmerged_entries`, `ci_passed`, `ci_pending`, every configured-reviewer row, +`checks_evidence`, `unresolved_review_threads`, `actionable_issue_comments_from_bots`, `claude_summary`, and +`review_evidence`, and `recommendation`. The parseable evidence marker is +`reviewer=<login|none> qualification=<qualified|blocked|unqualified|unknown> +reviewed_head_sha=<SHA|unknown> review_state=<state|none|unknown> +eligibility=<eligible|blocked|ineligible|unknown> +verdict=<approved|clean|blocker|dismissed|unknown|none> +trusted_producer=<true|false|unknown> +active_changes_requested=<N|unknown> +blocked_exact_current_head_reviews=<N|unknown>`. Emit a non-empty `ci_failed` list for observed failures, `ci_failed: unknown` when CI collection fails, and omit the entire field only when successful collection observed no failures. Use `unknown` for any other field whose collection fails. diff --git a/.cursor/agents/qa.md b/.cursor/agents/qa.md index bf9a22cafb..f4ed879c8b 100644 --- a/.cursor/agents/qa.md +++ b/.cursor/agents/qa.md @@ -1,6 +1,6 @@ --- name: qa -description: Verify integrated Kandev work by tracing wiring, exercising behavior, and probing edge cases. +description: Independently verify Kandev changes with integration, public-contract, persistence, concurrency, recovery, cross-component, or missing faithful behavior evidence. model: composer-2.5 readonly: true --- diff --git a/.cursor/agents/simplify.md b/.cursor/agents/simplify.md index 9ebc7c68bd..7625ddd79c 100644 --- a/.cursor/agents/simplify.md +++ b/.cursor/agents/simplify.md @@ -8,4 +8,4 @@ readonly: false Remove one-off abstractions, speculative scaffolding, unnecessary nesting, clever indirection, redundant validation, and noise only within the assigned diff. Run focused tests and report modified files and net simplification. -Do not spawn subagents. Leave full verification to the planner. +Do not spawn subagents. Leave final change-aware verification to the planner. diff --git a/.cursor/agents/verify.md b/.cursor/agents/verify.md index 623f635a1c..41a6ad99e4 100644 --- a/.cursor/agents/verify.md +++ b/.cursor/agents/verify.md @@ -1,15 +1,19 @@ --- name: verify -description: Run Kandev format, typecheck, tests, and lint, then report failures without fixing source logic. +description: Verify committed changes before push, reusing proven hook coverage and reporting full-mode escalation when needed. model: composer-2.5 readonly: false --- -Follow `.agents/agents/verify.md`. Run the complete quiet verification pipeline -and return targeted failure evidence for a new implementer assignment. Do not +Follow `.agents/agents/verify.md`. Run after commit and before push. Default to +changed scope; validate supplied hook evidence, run only uncovered matrix +commands, and report PR/scope bases, receipt eligibility, omissions, paths, +commands, and limits. Call a pass `changed-scope PASS` unless full mode was +triggered. An eligible hook-covered command must be omitted, not rerun for +reassurance. Do not fix production/test logic, rebase, or resolve conflicts. Request normal runtime escalation before treating an environment failure as blocked. If the required capability still cannot be authorized, include a required user action telling the user to enable Cursor's full filesystem, network, or loopback access as -needed, then retry verification. Do not offer an unverified PR. Do not spawn -subagents. +needed, then retry verification. Explain that push and PR delivery are waiting; +do not offer an unverified PR. Do not spawn subagents. diff --git a/.github/workflows/opencode-code-review.yml b/.github/workflows/opencode-code-review.yml index 4998beaa42..dd9c063d00 100644 --- a/.github/workflows/opencode-code-review.yml +++ b/.github/workflows/opencode-code-review.yml @@ -1,13 +1,8 @@ name: OpenCode Code Review -# Dual triggers: -# - pull_request: same-repo PRs run automatically. -# - pull_request_target: fork PRs can run only after maintainer opt-in. -# The fork path checks out the PR head but only lets OpenCode read files and -# post review comments through narrow `gh` permissions. +# Runs from the base-controlled workflow only. Both same-repository and fork +# paths inspect the immutable PR head without executing PR-owned workflow code. on: - pull_request: - types: [opened, synchronize, ready_for_review, reopened] pull_request_target: # `labeled` is needed so a maintainer applying `safe-to-review` to a fork # PR triggers a review against the current head SHA. Subsequent fork pushes @@ -25,14 +20,17 @@ jobs: opencode-review-same-repo: if: > vars.OPENCODE_REVIEW_ENABLED == 'true' && - github.event_name == 'pull_request' && + github.event_name == 'pull_request_target' && github.event.pull_request.draft == false && - github.event.pull_request.head.repo.full_name == github.repository + github.event.pull_request.head.repo.full_name == github.repository && + vars.OPENCODE_REVIEW_APP_CLIENT_ID != '' && + vars.OPENCODE_REVIEW_APP_SLUG != '' runs-on: ubuntu-latest timeout-minutes: 15 + environment: opencode-review-trusted permissions: contents: read - pull-requests: write + pull-requests: read steps: - name: Checkout PR head @@ -42,12 +40,16 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: Assert immutable PR head checkout + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + test "$(git rev-parse HEAD)" = "$HEAD_SHA" + - name: Build review inputs id: review-inputs env: - ACTION: ${{ github.event.action }} - BEFORE: ${{ github.event.before }} - AFTER: ${{ github.event.after }} BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | @@ -57,16 +59,8 @@ jobs: rm -rf "$REVIEW_DIR" mkdir -p "$REVIEW_DIR" - if [[ "$ACTION" == "synchronize" ]] && - [[ -n "${BEFORE:-}" ]] && - [[ "$BEFORE" != "0000000000000000000000000000000000000000" ]] && - git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then - RANGE="$BEFORE..${AFTER:-$HEAD_SHA}" - MODE="incremental" - else - RANGE="$BASE_SHA...$HEAD_SHA" - MODE="full-pr" - fi + RANGE="$BASE_SHA...$HEAD_SHA" + MODE="full-pr" git diff --find-renames --find-copies --unified=80 "$RANGE" > "$REVIEW_DIR/review.patch" git diff --name-only "$RANGE" > "$REVIEW_DIR/files.txt" @@ -189,12 +183,32 @@ jobs: echo "Output files are preserved under \`$REVIEW_DIR/\`." } >> "$GITHUB_STEP_SUMMARY" + - name: Create dedicated OpenCode review App token + id: app-token + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.OPENCODE_REVIEW_APP_CLIENT_ID }} + private-key: ${{ secrets.OPENCODE_REVIEW_ENV_APP_PRIVATE_KEY }} + permission-pull-requests: write + + - name: Assert dedicated OpenCode review App identity + env: + EXPECTED_APP_SLUG: ${{ vars.OPENCODE_REVIEW_APP_SLUG }} + ACTUAL_APP_SLUG: ${{ steps.app-token.outputs.app-slug }} + run: | + set -euo pipefail + test -n "$EXPECTED_APP_SLUG" + test -n "$ACTUAL_APP_SLUG" + test "$ACTUAL_APP_SLUG" = "$EXPECTED_APP_SLUG" + - name: Post OpenCode findings env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.app-token.outputs.token }} GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + GITHUB_RUN_ID: ${{ github.run_id }} + GITHUB_RUN_ATTEMPT: ${{ github.run_attempt }} BASE_SHA: ${{ github.event.pull_request.base.sha }} OPENCODE_MODEL: ${{ env.OPENCODE_MODEL }} run: | @@ -234,7 +248,7 @@ jobs: contains(github.event.pull_request.labels.*.name, 'safe-to-review') runs-on: ubuntu-latest permissions: - pull-requests: write + issues: write steps: - uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7 with: @@ -276,18 +290,20 @@ jobs: - name: Checkout PR head uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: - # pull_request_target checks out the base branch by default; explicitly - # check out the contributor's head via the base repository PR ref. - ref: refs/pull/${{ github.event.pull_request.number }}/head + ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 persist-credentials: false + - name: Assert immutable PR head checkout + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + test "$(git rev-parse HEAD)" = "$HEAD_SHA" + - name: Build review inputs id: review-inputs env: - ACTION: ${{ github.event.action }} - BEFORE: ${{ github.event.before }} - AFTER: ${{ github.event.after }} BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | @@ -298,16 +314,8 @@ jobs: mkdir -p "$REVIEW_DIR" git fetch --no-tags --prune origin "$BASE_SHA" - if [[ "$ACTION" == "synchronize" ]] && - [[ -n "${BEFORE:-}" ]] && - [[ "$BEFORE" != "0000000000000000000000000000000000000000" ]] && - git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then - RANGE="$BEFORE..${AFTER:-$HEAD_SHA}" - MODE="incremental" - else - RANGE="$BASE_SHA...$HEAD_SHA" - MODE="full-pr" - fi + RANGE="$BASE_SHA...$HEAD_SHA" + MODE="full-pr" git diff --find-renames --find-copies --unified=80 "$RANGE" > "$REVIEW_DIR/review.patch" git diff --name-only "$RANGE" > "$REVIEW_DIR/files.txt" @@ -432,10 +440,13 @@ jobs: - name: Post OpenCode findings env: + # Advisory fork evidence is deliberately nonqualifying. GH_TOKEN: ${{ github.token }} GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + GITHUB_RUN_ID: ${{ github.run_id }} + GITHUB_RUN_ATTEMPT: ${{ github.run_attempt }} BASE_SHA: ${{ github.event.pull_request.base.sha }} OPENCODE_MODEL: ${{ env.OPENCODE_MODEL }} run: | diff --git a/.opencode/agents/architect.md b/.opencode/agents/architect.md index d24e0c14e4..ab65fb838d 100644 --- a/.opencode/agents/architect.md +++ b/.opencode/agents/architect.md @@ -1,5 +1,5 @@ --- -description: Provide a frontier-model second opinion on unusually risky Kandev architecture and planning; the primary planner owns normal specs and plans. +description: User-requested independent frontier second opinion; planner owns normal architecture, specs, and plans. mode: subagent temperature: 0.1 permission: diff --git a/.opencode/agents/code-review.md b/.opencode/agents/code-review.md index ec1f1122f5..2c8e02e9ca 100644 --- a/.opencode/agents/code-review.md +++ b/.opencode/agents/code-review.md @@ -1,5 +1,5 @@ --- -description: Review changed Kandev code for correctness, security, architecture, scope, complexity, and missing tests after implementation or before PRs. +description: Frontier-review Kandev changes with architecture, cross-cutting, high-risk, or stale/incomplete external-review risk. mode: subagent temperature: 0.1 permission: diff --git a/.opencode/agents/pr-poller.md b/.opencode/agents/pr-poller.md index 4420afb907..8c7e7b42e0 100644 --- a/.opencode/agents/pr-poller.md +++ b/.opencode/agents/pr-poller.md @@ -29,11 +29,32 @@ Include head SHA, GitHub mergeability/merge-state status, and the local unmerged index count from `git ls-files -u`. Use one-shot checks or bounded commands. Report only observed values and return one compact report block. +The planner may provide `selected_reviewer=<GitHub login>`. For selection, +read raw `scripts/pr-state --trusted-reviewer "$selected_reviewer" <PR>`: only known/complete records with matching +checks/opening/closing heads, `checks_snapshot_complete=true`, and +`commit_id == evidence_head_sha` qualify; H1 checks with H2 reviews must retry +or fall back. Timestamps never establish head coverage. Only `eligibility=eligible` (approved with no blocker or explicit +`<!-- kandev-review: clean -->`) qualifies; selected `${OPENCODE_REVIEW_APP_SLUG}[bot]` must +also have emitted `trusted_producer=true` from `scripts/pr-state --trusted-reviewer "$selected_reviewer" <PR>` on the exact-head record; only the dedicated OpenCode App requires this provenance, while other selected reviewers retain generic exact-head qualification. Generic +marker reviews do not qualify. Ambiguous/dismissed/pending/unknown +reviews do not. Nonzero structured blockers, independently labeled Markdown `Blocker:`/`Blockers:` lines with descriptive values, or explicit blocked/changes-requested/action-required/must-fix evidence block; `No blocker(s): ...`, `Blocker(s): 0`, and `| Blocker | 0 |` do not. Once CI is complete, exit on qualified selected evidence or an +observed blocker (active change request, any +`blocked_exact_current_head_review_count > 0`, `eligibility=blocked`, or +actionable unresolved thread) without waiting for unrelated bots. Without a +selected reviewer, use the normal configured-reviewer terminal condition. + Follow the exact output contract in `.agents/agents/pr-poller.md`: delimit the only output with `=== pr-poller report ===` and `=== end ===`, and include `pr`/`branch`, `head_sha`, `mergeable`, `merge_state_status`, -`local_unmerged_entries`, `ci_failed`, `ci_passed`, `ci_pending`, every bot row, -`unresolved_review_threads`, `issue_comments_from_bots`, `claude_summary`, and -`recommendation`. Use `unknown` when collection fails. +`local_unmerged_entries`, `ci_failed`, `ci_passed`, `ci_pending`, every configured-reviewer row, +`checks_evidence`, `unresolved_review_threads`, `actionable_issue_comments_from_bots`, `claude_summary`, and +`review_evidence`, and `recommendation`. Emit `review_evidence:` as +`reviewer=<login|none> qualification=<qualified|blocked|unqualified|unknown> +reviewed_head_sha=<SHA|unknown> review_state=<state|none|unknown> +eligibility=<eligible|blocked|ineligible|unknown> +verdict=<approved|clean|blocker|dismissed|unknown|none> +trusted_producer=<true|false|unknown> +active_changes_requested=<N|unknown> +blocked_exact_current_head_reviews=<N|unknown>`. Use `unknown` when collection fails. Do not spawn subagents. diff --git a/.opencode/agents/qa.md b/.opencode/agents/qa.md index cbb32869d0..d7658d276a 100644 --- a/.opencode/agents/qa.md +++ b/.opencode/agents/qa.md @@ -1,5 +1,5 @@ --- -description: Verify integrated Kandev work after implementation by tracing wiring, testing happy paths, breaking edge cases, and checking coverage/readiness. +description: Independently verify Kandev changes with integration, public-contract, persistence, concurrency, recovery, cross-component, or missing faithful behavior evidence. mode: subagent temperature: 0.1 permission: diff --git a/.opencode/agents/simplify.md b/.opencode/agents/simplify.md index 404692d728..6dadf21ec2 100644 --- a/.opencode/agents/simplify.md +++ b/.opencode/agents/simplify.md @@ -13,4 +13,4 @@ Review recently changed files and simplify while preserving behavior. Remove one Verify after meaningful simplifications. If behavior changes, revert that simplification. Report modified files, what changed, and verification. -Do not spawn subagents. Report required full verification to the planner. +Do not spawn subagents. Report required change-aware verification to the planner. diff --git a/.opencode/agents/verify.md b/.opencode/agents/verify.md index 34c0d01da1..75a286a908 100644 --- a/.opencode/agents/verify.md +++ b/.opencode/agents/verify.md @@ -1,5 +1,5 @@ --- -description: Run Kandev format, typecheck, tests, and lint before commit, then report failures without fixing source or test logic. +description: Verify committed changes before push, reusing proven hook coverage and using full mode only for broad or ambiguous impact. mode: subagent temperature: 0.1 permission: @@ -9,16 +9,20 @@ permission: "*": ask --- -Run the monorepo verification pipeline and report issues found. +Follow `.agents/agents/verify.md`, its impact matrix, and hook-evidence +reference. Run after commit and before push. Default to changed scope; validate +any supplied receipt, run only uncovered commands, and report PR/scope bases, +receipt eligibility, omissions, changed paths, commands, limits, and +`changed-scope PASS` versus `full PASS`. Never infer hook success, and never +rerun an eligible hook-covered command for reassurance. Install `apps` dependencies when missing. Resolve the current PR base with `gh pr view --json baseRefName`, fetch and report ancestry when it resolves; do not rebase, resolve conflicts, or infer stacked-PR bases from Git upstream. -Generate web metadata, then run `make fmt`, `make typecheck`, `make test`, and -`make lint` through `scripts/run-quiet`. Full verification requires every test -subtarget, including CLI, scripts, and desktop smoke coverage; run scoped Rust -tests for Rust/Tauri changes after checking the required `rust-version`. +Run only uncovered matrix-selected commands. Full mode is for explicit, +ambiguous, broad, release/toolchain, or no-PR-CI delivery; ignore hook +omissions and cover every relevant subtarget. Do not fix source or test logic. Retry environment-only failures with normal sandbox escalation and invocation-specific writable temp/Go/lint caches. For @@ -27,7 +31,7 @@ an implementer. Finish with a compact pass/fail report. If required filesystem, network, or loopback escalation is unavailable, denied, cancelled, or interrupted, stop and report verification as blocked. Explain -that mandatory verification is preventing commit and PR creation. Include a +that mandatory verification is preventing push and PR delivery. Include a required user action telling the user to enable the runtime's full access mode, then retry verification. Do not offer to proceed unverified or imply that the agent or repository host cannot create PRs. Recommend full access only after diff --git a/AGENTS.md b/AGENTS.md index e75797bcee..be590b1374 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,7 +75,10 @@ Every code change must include tests for new or changed logic. Backend: `*_test. - **Plans:** Implementation plans are generated from specs via `/plan` and committed under `docs/plans/<slug>/plan.md`, with individual sibling task files named `docs/plans/<slug>/task-<NN>-<short-slug>.md`. Specs are the living requirements; plans and task files are implementation records for the current buildout. ### Plan Implementation -- After implementing a plan, run `make fmt` first to format code, then run `make typecheck test lint` to verify the changes. Formatting must come first because formatters may split lines, which can trigger complexity linter warnings. +- After implementing a substantial plan, commit through active hooks, then + delegate `verify` with `mode=full` before push. Full mode ignores hook + omissions and runs `make fmt` before `make typecheck test lint`; formatting + comes first because it may split lines and expose complexity-linter failures. ### Observability - In dev mode (`KANDEV_MOCK_AGENT=true` or `debug.pprofEnabled`), `/debug/vars` exposes the stdlib expvar handler. Office provider-routing metrics live under `routing_*` (route attempts, fallbacks, parked runs, provider degraded/recovered counters). The metrics are also still emitted as structured `routing.metric.*` zap logs for human debugging. @@ -93,17 +96,25 @@ When a Kandev system message references an MCP tool that is not visible in the a ### Planner and Worker Execution -The user-started primary session is the planner. It may clarify intent, read -repository context, create planning artifacts, delegate bounded work, review -results, and report status. It must not implement or edit application, test, or -harness code; run tests or verification; commit or push changes; or create PRs. +The user-started primary session is the planner and default architect: it owns +clarification, architecture, specs, plans, task decomposition, integration +judgment, and user communication. It may directly perform small scoped work +when one clear concern touches a few localized files, has no useful isolation or +parallelism benefit, and has quick bounded verification. Follow the applicable +skill, protect unrelated dirty changes, and obtain delegated Spark `verify` +after commit and before push when code, tests, or config changed. Pass the +successful non-bypassed hook receipt so changed-scope verification skips only +equivalent hook-covered checks; PR CI remains the authoritative full matrix. -All execution must be delegated through the current coding harness's native -subagent tools. Never use Kandev MCP task/session APIs as a delegation fallback. -Each delegated worker executes one bounded work packet and must not spawn other -agents. If native delegation is unavailable, stop and report that limitation. -Detailed work-packet and model-selection rules live in the -`planner-orchestration` skill. +Delegate only when it has positive ROI or independent evidence is essential: +broad/unknown exploration, substantial plan tasks, large/cross-component work, +parallel packets, long/noisy E2E or debugging, exceptional specialist review, +and final change-aware `verify`. Keep long PR monitoring on cheap `pr-poller`. +Delegation is not default ceremony: weigh context reload and coordination cost. +Each delegated worker executes one bounded packet and does not spawn agents. +Never use Kandev MCP task/session APIs as a delegation fallback. The architect +agent is only for a user-requested independent architecture second opinion. +Detailed routing lives in `planner-orchestration`. ### Kandev Task Creation diff --git a/docs/decisions/2026-07-23-opencode-review-evidence-trust.md b/docs/decisions/2026-07-23-opencode-review-evidence-trust.md new file mode 100644 index 0000000000..7e52aba1f9 --- /dev/null +++ b/docs/decisions/2026-07-23-opencode-review-evidence-trust.md @@ -0,0 +1,42 @@ +# ADR-2026-07-23-opencode-review-evidence-trust: Trusted OpenCode Review Evidence + +**Status:** accepted +**Date:** 2026-07-23 +**Area:** workflow, infra + +## Context + +Routine PR review can use OpenCode running inside a trusted base-commit wrapper, but GitHub records `github-actions[bot]` for every shared workflow identity. That identity is forgeable by unrelated Actions workflows, and incremental review cannot prove full-PR coverage. Fork-safe workflows also need review evidence without granting untrusted PR code write authority. + +## Decision + +Trusted jobs run only from the base-controlled `pull_request_target` workflow, check out and assert the immutable head SHA, use only `BASE_SHA...HEAD_SHA`, and load the publisher from `BASE_SHA`; no App credential reaches the model step. Only same-repository review attaches the protected `opencode-review-trusted` environment and mints the dedicated current-repository-only installed GitHub App pull-request-write token; it uses `OPENCODE_REVIEW_ENV_APP_PRIVATE_KEY`, created only in that environment with no repository/org fallback secret, and administrators restrict environment deployment branches to the protected default branch. Its login is `${OPENCODE_REVIEW_APP_SLUG}[bot]`. Fork review never attaches this environment, remains advisory under the base-controlled workflow's GitHub token, and can never qualify. + +The wrapper accepts exactly one findings block covering the whole trimmed model output (an optional JSON fence is allowed inside it). Valid empty output emits the wrapper clean marker in an exact-head `COMMENT` review; valid findings emit an action-required terminal review. Every malformed, missing, or failed model result attempts an exact-head non-clean diagnostic review and fails the job. A same-head non-clean review remains blocking even if a later clean review appears; only a new head can recover. Historical no-findings issue comments are non-actionable, while diagnostic and fallback comments remain actionable. + +Every trusted-wrapper terminal review (clean, blocked, or diagnostic) includes a machine marker containing its exact `GITHUB_RUN_ID` and `GITHUB_RUN_ATTEMPT`; the clean marker remains a separate wrapper-owned semantic marker. `scripts/pr-state` grants `trusted_default_producer` only to an exact-head clean COMMENTED review whose author exactly equals the explicit configured App login, whose marker matches the authenticated latest workflow run and attempt, and whose PR is explicitly known same-repository. It queries only `actions/workflows/opencode-code-review.yml/runs`, requires the base-controlled workflow path, `pull_request_target` event, PR/head association, and selects the latest `(run_number, run_attempt, id)` generation. It then requires the exact attempt's `opencode-review-same-repo` job plus run conclusion to be completed/success, and performs its authoritative final head read only after those Actions API calls. Display names and check rollups never authenticate evidence. The canonical emitted `trusted_producer` is `true` only when the raw marker/App predicate and successful authenticated workflow/job predicate both pass, `false` for known failure, and `unknown` for incomplete/API state. Missing or failed API reads, marker mismatch, a post-API head change, and pending, cancelled, skipped, neutral, or failed latest attempts invalidate old clean evidence. This shortcut is limited to routine same-repository work with successful full-PR evidence and all existing check, thread, and blocker gates. + +External setup is required: `OPENCODE_REVIEW_ENABLED=true`, App client ID, App slug, the protected environment-only private-key secret, current-repository-only installation, and Pull requests write permission. Missing or misconfigured environment protection remains fail-closed. This repository does not mutate GitHub settings. + +For PR delivery, qualifying trusted evidence is the default semantic path to +avoid spending local frontier-review capacity on routine work. Local review is +exceptional: unusually large/complex/cross-cutting architecture, terminal +readiness that cannot wait for contradictory or repeatedly unavailable evidence, +an explicit request, or a deep automated-review gap. Security auditing is +likewise limited to high-impact new/changed authz, workspace-isolation, secrets, +untrusted-execution, or credential-trust boundaries, explicit request, or +concrete automated security concerns. + +No product spec applies because this is workflow and infrastructure evidence policy. + +## Consequences + +Selected OpenCode evidence can end polling without unrelated bots once CI and all blocker gates are clean, while generic Actions activity cannot satisfy the gate. COMMENT deliberately avoids representing automated review as human approval. This PR-first default reduces token cost while preserving fail-closed exact-head qualification; a small scope-preserving fix uses its finding, focused regression, final verification, and fresh exact-head review rather than restarting every local gate. The App introduces credentials, installation, and operational setup; unavailable setup uses the exceptional local-review route when terminal readiness requires it. + +## Alternatives Considered + +- **Claude or Greptile as the routine producer:** useful independent reviewers, but their availability, identity, and delivery paths do not provide this trusted OpenCode-wrapper contract. +- **Local-only frontier review:** rejected as the routine path because it consumes local review capacity and tokens; retained as the exceptional fallback. +- **Shared Actions identity:** rejected because any Actions workflow can impersonate `github-actions[bot]`. +- **Check-correlation-only evidence:** rejected as complex and incomplete without a dedicated review identity and terminal semantic review. +- **Dedicated GitHub App:** chosen despite credentials, installation, and operational ownership because it provides a non-forgeable producer identity. diff --git a/docs/decisions/2026-07-23-planner-direct-small-work.md b/docs/decisions/2026-07-23-planner-direct-small-work.md new file mode 100644 index 0000000000..9078078849 --- /dev/null +++ b/docs/decisions/2026-07-23-planner-direct-small-work.md @@ -0,0 +1,40 @@ +# Planner Direct Small Work + +**Status:** accepted +**Date:** 2026-07-23 +**Area:** workflow + +## Context + +Delegating every action reloads context and adds coordination/token cost, even +when one planner can safely complete a localized change faster. + +## Decision + +The planner is the default product and technical architect. It directly handles +one clear concern with a few localized files, no meaningful isolation or +parallelism benefit, and quick targeted verification. It delegates broad or +unknown exploration, substantial plan tasks, large/cross-component work, +parallel packets, long/noisy E2E or debugging, exceptional specialist review, +and final Spark `verify`; long PR monitoring stays with cheap `pr-poller`. +Architect is only a user-requested independent second opinion. + +Spark `verify` defaults to changed-scope suites selected from the diff; GitHub +PR CI is the authoritative full matrix. Full local verification remains for +explicit requests, releases/shared toolchain changes, ambiguous impact, broad +cross-component work, or delivery without PR CI. + +## Consequences + +Small work avoids avoidable context reload while retaining skills, TDD, +dirty-worktree safety, focused checks, and final Spark verification for +code/test/config changes. Delegated workers retain one-packet/no-subagent +isolation. No product spec applies. + +## Alternatives Considered + +- **Delegate everything:** rejected for routine context and coordination cost. +- **Planner does everything:** rejected because independent evidence and long + noisy work still benefit from specialization. +- **Always run full local verification:** rejected for routine cost; retained + for broad/ambiguous or no-CI delivery. diff --git a/docs/decisions/2026-07-23-post-commit-hook-aware-verification.md b/docs/decisions/2026-07-23-post-commit-hook-aware-verification.md new file mode 100644 index 0000000000..5cf3f9b5cc --- /dev/null +++ b/docs/decisions/2026-07-23-post-commit-hook-aware-verification.md @@ -0,0 +1,50 @@ +# ADR-2026-07-23-post-commit-hook-aware-verification: Post-Commit Hook-Aware Verification + +**Status:** accepted +**Date:** 2026-07-23 +**Area:** workflow + +## Context + +Kandev's pre-commit hooks already format changed Go and TypeScript files and +lint changed backend, web, and harness files. Running the same checks again in +local verification adds latency and agent cost, while tests, typechecks, and +specialized validators still need an independent gate. + +## Decision + +Final local verification runs after commit and before push. The commit workflow +returns a hook receipt containing the commit SHA, active-hook status, absence +of bypass flags or environment variables, successful commit result, and +post-commit worktree state. + +In changed-scope mode with authoritative PR CI, `verify` must omit checks whose +changed paths and behavior are exactly covered by that receipt, and may omit no +others. It first +confirms that the receipt matches current `HEAD`, the verification delta, and a +clean worktree. Tests, typechecks, generated-metadata checks, script/docs/ +workflow validators, TOML parsing, and other uncovered checks still run. + +Missing, stale, bypassed, partial, or ambiguous hook evidence disables the +optimization. Full verification and delivery without PR CI never omit checks +because of hook evidence. A later edit or formatter change invalidates the +receipt and requires a new commit and verification. + +No product spec applies because this is an internal delivery convention. + +## Consequences + +Routine work avoids repeating changed-file formatting and linting while +retaining post-commit verification of the exact artifact that will be pushed. +The planner must preserve a small hook receipt and last verified SHA between +commit, verification, and push. Verification remains fail-closed when that +handoff cannot be proven. + +## Alternatives Considered + +- **Verify before commit:** rejected because hooks repeat checks and may change + files after verification. +- **Always rerun hook-covered checks:** rejected because PR CI already supplies + the authoritative full matrix and the duplicate local work has low value. +- **Rely only on hooks and PR CI:** rejected because hooks do not cover tests, + typechecks, or several specialized validators. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index c8311748e0..0933b22f39 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -56,6 +56,9 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 0046 | [Settings route save coordinator](0046-settings-route-save-coordinator.md) | accepted | frontend | 2026-07-14 | | 0047 | [Plugins read conversation content via a capability-gated Host RPC](0047-plugin-host-conversation-reads.md) | accepted | backend, protocol | 2026-07-21 | | 0048 | [Plugins invoke a settings-selectable utility agent](0048-plugin-host-utility-agent-invoke.md) | accepted | backend, frontend, protocol | 2026-07-21 | +| 2026-07-23-opencode-review-evidence-trust | [Trusted OpenCode Review Evidence](2026-07-23-opencode-review-evidence-trust.md) | accepted | workflow, infra | 2026-07-23 | +| 2026-07-23-planner-direct-small-work | [Planner Direct Small Work](2026-07-23-planner-direct-small-work.md) | accepted | workflow | 2026-07-23 | +| 2026-07-23-post-commit-hook-aware-verification | [Post-Commit Hook-Aware Verification](2026-07-23-post-commit-hook-aware-verification.md) | accepted | workflow | 2026-07-23 | | 2026-07-18-turn-configuration-snapshots | [Attribute runtime configuration to turns](2026-07-18-turn-configuration-snapshots.md) | accepted | backend, frontend | 2026-07-18 | | 2026-07-19-reject-mcp-actions-on-raw-websocket | [Reject MCP Actions on the Raw WebSocket](2026-07-19-reject-mcp-actions-on-raw-websocket.md) | accepted | backend, protocol | 2026-07-19 | | 2026-07-19-workspace-symlink-entries | [Treat Nested Workspace Symlinks as Entries](2026-07-19-workspace-symlink-entries.md) | accepted | backend, infra | 2026-07-19 | diff --git a/scripts/opencode-code-review b/scripts/opencode-code-review index 60fd59a4a0..1fd6da170d 100755 --- a/scripts/opencode-code-review +++ b/scripts/opencode-code-review @@ -15,6 +15,13 @@ OMISSION_NOTICE_RESERVE = 256 NO_FINDINGS_MARKER = "<!-- opencode-review:no-findings -->" DIAGNOSTIC_MARKER = "<!-- opencode-review:diagnostic -->" FALLBACK_MARKER = "<!-- opencode-review:fallback-findings -->" +TRUSTED_CLEAN_MARKER = "<!-- kandev-review: clean -->" + + +def workflow_run_marker() -> str: + run_id = required_env("GITHUB_RUN_ID") + attempt = required_env("GITHUB_RUN_ATTEMPT") + return f"<!-- kandev-review: workflow-run id={run_id} attempt={attempt} -->" def required_env(name: str) -> str: @@ -108,7 +115,7 @@ def read_status(path: Path) -> int: def parse_findings(raw: str) -> tuple[list[Any] | None, str | None]: - match = re.search(r"<opencode_findings>\s*(.*?)\s*</opencode_findings>", raw, re.S) + match = re.fullmatch(r"\s*<opencode_findings>\s*(.*?)\s*</opencode_findings>\s*", raw, re.S) if not match: return None, "No parseable findings block was found." payload_text = match.group(1).strip() @@ -158,6 +165,49 @@ def no_findings_body() -> str: ) +def terminal_review_body(*, clean: bool, finding_count: int) -> str: + short_sha = required_env("HEAD_SHA")[:12] + model = required_env("OPENCODE_MODEL") + if clean: + return ( + f"{TRUSTED_CLEAN_MARKER}\n{workflow_run_marker()}\n" + "**OpenCode review complete**\n\n" + "Trusted wrapper parsed an empty findings array.\n\n" + f"OpenCode model: `{model}`\n" + f"Commit: `{short_sha}`" + ) + return ( + f"{workflow_run_marker()}\n**OpenCode review requires action**\n\n" + f"Trusted wrapper parsed `{finding_count}` finding(s); action required before this review can be clean.\n\n" + f"OpenCode model: `{model}`\n" + f"Commit: `{short_sha}`" + ) + + +def post_terminal_review(*, clean: bool, finding_count: int) -> bool: + repository = required_env("GITHUB_REPOSITORY") + pr_number = required_env("PR_NUMBER") + head_sha = required_env("HEAD_SHA") + result = gh_api( + [ + "-X", + "POST", + f"repos/{repository}/pulls/{pr_number}/reviews", + "-f", + f"commit_id={head_sha}", + "-f", + "event=COMMENT", + "-f", + f"body={terminal_review_body(clean=clean, finding_count=finding_count)}", + ] + ) + if result.returncode == 0: + print("Posted OpenCode terminal pull-request review.") + return True + print(f"Could not post OpenCode terminal review: {result.stderr.strip()}", file=sys.stderr) + return False + + def fallback_body(rejected: list[dict[str, Any]], overflow: list[dict[str, Any]]) -> tuple[str, int, int]: model = required_env("OPENCODE_MODEL") short_sha = required_env("HEAD_SHA")[:12] @@ -326,9 +376,12 @@ def handle_diagnostic( *snippet("OpenCode findings output", raw), ], ) + posted_terminal_review = post_terminal_review(clean=False, finding_count=0) upsert_issue_comment(DIAGNOSTIC_MARKER, diagnostic_body(reason, status, stdout, stderr)) print(f"OpenCode did not produce parseable findings: {reason}", file=sys.stderr) - return 1 if fail_check else 0 + if not posted_terminal_review: + print("OpenCode terminal diagnostic review could not be posted.", file=sys.stderr) + return 1 def post_findings(args: argparse.Namespace) -> int: @@ -357,11 +410,11 @@ def post_findings(args: argparse.Namespace) -> int: stdout, stderr, raw, - fail_check=False, + fail_check=True, ) if not findings: - posted_no_findings = upsert_issue_comment(NO_FINDINGS_MARKER, no_findings_body()) + posted_terminal_review = post_terminal_review(clean=True, finding_count=0) append_summary( args.summary, "OpenCode review", @@ -372,7 +425,7 @@ def post_findings(args: argparse.Namespace) -> int: *snippet("OpenCode stderr", stderr), ], ) - return 0 if posted_no_findings else 1 + return 0 if posted_terminal_review else 1 changed_paths = set(args.files.read_text(encoding="utf-8").splitlines()) commentable_lines = None @@ -427,6 +480,8 @@ def post_findings(args: argparse.Namespace) -> int: fallback_comment, fallback_included, fallback_omitted = fallback_body(unplaced_findings, overflow_findings) posted_fallback = upsert_issue_comment(FALLBACK_MARKER, fallback_comment) + posted_terminal_review = post_terminal_review(clean=False, finding_count=len(findings)) + summary_lines = [ f"Parsed findings: `{len(findings)}`", f"Inline comments posted: `{posted}`", @@ -441,7 +496,7 @@ def post_findings(args: argparse.Namespace) -> int: summary_lines.extend(["", *snippet("OpenCode stdout", stdout), *snippet("OpenCode stderr", stderr)]) append_summary(args.summary, "OpenCode review", summary_lines) print(f"Posted {posted} OpenCode inline comments; preserved {fallback_included} fallback findings.") - return 0 if posted_fallback else 1 + return 0 if posted_fallback and posted_terminal_review else 1 def build_parser() -> argparse.ArgumentParser: diff --git a/scripts/opencode-code-review.test.py b/scripts/opencode-code-review.test.py index 7276dfd80f..a30479cda6 100755 --- a/scripts/opencode-code-review.test.py +++ b/scripts/opencode-code-review.test.py @@ -59,9 +59,12 @@ class OpenCodeReviewScriptTest(unittest.TestCase): raise SystemExit(0) joined = " ".join(args) - if "/pulls/" in joined and os.environ.get("INLINE_FAIL") == "1": + if "/pulls/" in joined and "/comments" in joined and os.environ.get("INLINE_FAIL") == "1": print("line is not part of the diff", file=sys.stderr) raise SystemExit(1) + if "/reviews" in joined and os.environ.get("TERMINAL_REVIEW_FAIL") == "1": + print("terminal review unavailable", file=sys.stderr) + raise SystemExit(1) if "/issues/" in joined and os.environ.get("ISSUE_COMMENT_FAIL") == "1": print("issue comments unavailable", file=sys.stderr) raise SystemExit(1) @@ -80,9 +83,11 @@ class OpenCodeReviewScriptTest(unittest.TestCase): output: str, inline_fail: bool = False, issue_comment_fail: bool = False, + terminal_review_fail: bool = False, gh_bin: Path | None = None, include_patch: bool = False, ) -> subprocess.CompletedProcess[str]: + self.calls_path.unlink(missing_ok=True) self.output_path.write_text(output, encoding="utf-8") env = { **os.environ, @@ -91,12 +96,16 @@ class OpenCodeReviewScriptTest(unittest.TestCase): "GITHUB_REPOSITORY": "kdlbs/kandev", "PR_NUMBER": "42", "HEAD_SHA": "0123456789abcdef", + "GITHUB_RUN_ID": "27340000005", + "GITHUB_RUN_ATTEMPT": "2", "OPENCODE_MODEL": "opencode-go/minimax-m3", } if inline_fail: env["INLINE_FAIL"] = "1" if issue_comment_fail: env["ISSUE_COMMENT_FAIL"] = "1" + if terminal_review_fail: + env["TERMINAL_REVIEW_FAIL"] = "1" command = [ sys.executable, str(SCRIPT), @@ -137,10 +146,68 @@ class OpenCodeReviewScriptTest(unittest.TestCase): bodies.append(call[index + 1][len("body=") :]) return bodies - def test_missing_findings_block_posts_diagnostic_without_failing(self) -> None: + def terminal_review_calls(self) -> list[list[str]]: + return [call for call in self.read_calls() if any("/reviews" in arg for arg in call)] + + def terminal_review_body(self, call: list[str]) -> str: + return next(arg[len("body=") :] for arg in call if arg.startswith("body=")) + + def test_empty_findings_post_trusted_exact_head_clean_terminal_review(self) -> None: + result = self.run_script(output="<opencode_findings>[]</opencode_findings>\n") + + self.assertEqual(result.returncode, 0, result.stderr) + terminal_calls = self.terminal_review_calls() + self.assertEqual(len(terminal_calls), 1, terminal_calls) + call = terminal_calls[0] + self.assertIn("repos/kdlbs/kandev/pulls/42/reviews", call) + self.assertIn("commit_id=0123456789abcdef", call) + self.assertIn("event=COMMENT", call) + body = self.terminal_review_body(call) + self.assertIn("<!-- kandev-review: clean -->", body) + self.assertIn("<!-- kandev-review: workflow-run id=27340000005 attempt=2 -->", body) + self.assertIn("OpenCode review complete", body) + + def test_nonempty_findings_post_blocked_terminal_review_without_clean_marker(self) -> None: + findings = [{"path": "src/app.ts", "line": 99, "title": "Bad line", "body": "This line moved."}] + result = self.run_script(output=f"<opencode_findings>{json.dumps(findings)}</opencode_findings>\n") + + self.assertEqual(result.returncode, 0, result.stderr) + terminal_calls = self.terminal_review_calls() + self.assertEqual(len(terminal_calls), 1, terminal_calls) + body = self.terminal_review_body(terminal_calls[0]) + self.assertNotIn("<!-- kandev-review: clean -->", body) + self.assertIn("<!-- kandev-review: workflow-run id=27340000005 attempt=2 -->", body) + self.assertIn("action required", body.lower()) + + def test_diagnostic_paths_never_post_clean_terminal_review(self) -> None: + for output, status in [("not findings", "0\n"), ("<opencode_findings>[]</opencode_findings>", "2\n")]: + with self.subTest(output=output, status=status): + self.status_path.write_text(status, encoding="utf-8") + result = self.run_script(output=output) + self.assertNotEqual(result.returncode, 0) + terminal_calls = self.terminal_review_calls() + self.assertEqual(len(terminal_calls), 1, terminal_calls) + self.assertNotIn("<!-- kandev-review: clean -->", self.terminal_review_body(terminal_calls[0])) + self.assertIn("<!-- kandev-review: workflow-run id=27340000005 attempt=2 -->", self.terminal_review_body(terminal_calls[0])) + + def test_terminal_review_failure_fails_the_step(self) -> None: + result = self.run_script( + output="<opencode_findings>[]</opencode_findings>\n", terminal_review_fail=True + ) + + self.assertNotEqual(result.returncode, 0) + self.assertIn("Could not post OpenCode terminal review", result.stderr) + + def test_diagnostic_terminal_review_failure_fails_the_step(self) -> None: + result = self.run_script(output="not findings", terminal_review_fail=True) + + self.assertNotEqual(result.returncode, 0) + self.assertIn("Could not post OpenCode terminal review", result.stderr) + + def test_missing_findings_block_posts_diagnostic_and_fails(self) -> None: result = self.run_script(output="OpenCode refused to read external_directory (/tmp/*)\n") - self.assertEqual(result.returncode, 0) + self.assertNotEqual(result.returncode, 0) self.assertIn("did not produce parseable findings", result.stderr) self.assertTrue( any("<!-- opencode-review:diagnostic -->" in body for body in self.bodies()), @@ -148,10 +215,10 @@ class OpenCodeReviewScriptTest(unittest.TestCase): ) self.assertIn("No parseable findings block", self.summary_path.read_text()) - def test_invalid_findings_json_posts_diagnostic_without_failing(self) -> None: + def test_invalid_findings_json_posts_diagnostic_and_fails(self) -> None: result = self.run_script(output="<opencode_findings>{}</opencode_findings>\n") - self.assertEqual(result.returncode, 0) + self.assertNotEqual(result.returncode, 0) self.assertIn("did not produce parseable findings", result.stderr) self.assertTrue(any("<!-- opencode-review:diagnostic -->" in body for body in self.bodies())) self.assertIn("not an array", self.summary_path.read_text()) @@ -160,7 +227,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): result = self.run_script(output="<opencode_findings>```json\n[]\n```</opencode_findings>\n") self.assertEqual(result.returncode, 0, result.stderr) - self.assertTrue(any("<!-- opencode-review:no-findings -->" in body for body in self.bodies())) + self.assertTrue(any("<!-- kandev-review: clean -->" in body for body in self.bodies())) def test_missing_gh_binary_reports_gracefully(self) -> None: result = self.run_script( @@ -171,14 +238,29 @@ class OpenCodeReviewScriptTest(unittest.TestCase): self.assertNotEqual(result.returncode, 0) self.assertIn("Binary not found", result.stderr) - def test_valid_empty_array_posts_no_findings_comment(self) -> None: + def test_valid_empty_array_does_not_post_legacy_no_findings_issue_comment(self) -> None: result = self.run_script(output="<opencode_findings>[]</opencode_findings>\n") self.assertEqual(result.returncode, 0, result.stderr) bodies = self.bodies() - self.assertTrue(any("<!-- opencode-review:no-findings -->" in body for body in bodies)) + self.assertFalse(any("<!-- opencode-review:no-findings -->" in body for body in bodies)) self.assertFalse(any("<!-- opencode-review:diagnostic -->" in body for body in bodies)) + def test_parse_findings_rejects_prefix_suffix_duplicate_and_empty_then_nonempty_blocks(self) -> None: + invalid_outputs = [ + "prefix <opencode_findings>[]</opencode_findings>", + "<opencode_findings>[]</opencode_findings> suffix", + "<opencode_findings>[]</opencode_findings><opencode_findings>[]</opencode_findings>", + "<opencode_findings>[]</opencode_findings><opencode_findings>[{\"path\":\"src/app.ts\",\"line\":1,\"title\":\"bad\",\"body\":\"bad\"}]</opencode_findings>", + ] + for output in invalid_outputs: + with self.subTest(output=output): + result = self.run_script(output=output) + self.assertNotEqual(result.returncode, 0) + terminal_calls = self.terminal_review_calls() + self.assertEqual(len(terminal_calls), 1, terminal_calls) + self.assertNotIn("<!-- kandev-review: clean -->", self.terminal_review_body(terminal_calls[0])) + def test_inline_failures_are_posted_as_one_fallback_comment(self) -> None: result = self.run_script( output=textwrap.dedent( @@ -224,7 +306,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): ) self.assertEqual(result.returncode, 0, result.stderr) - pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg and "/comments" in arg for arg in call)] self.assertEqual(len(pull_comment_calls), 2, pull_comment_calls) self.assertTrue(any("line=9" in arg for call in pull_comment_calls for arg in call)) self.assertTrue(any("line=10" in arg for call in pull_comment_calls for arg in call)) @@ -259,7 +341,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): ) self.assertEqual(result.returncode, 0, result.stderr) - pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg and "/comments" in arg for arg in call)] self.assertEqual(len(pull_comment_calls), 1, pull_comment_calls) self.assertTrue(any("line=10" in arg for arg in pull_comment_calls[0])) self.assertFalse(any("line=99" in arg for call in self.read_calls() for arg in call)) @@ -296,7 +378,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): ) self.assertEqual(result.returncode, 0, result.stderr) - pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg and "/comments" in arg for arg in call)] self.assertEqual(len(pull_comment_calls), 1, pull_comment_calls) self.assertTrue(any("line=9" in arg for arg in pull_comment_calls[0])) self.assertFalse(any("line=10" in arg for call in self.read_calls() for arg in call)) diff --git a/scripts/opencode-code-review.test.sh b/scripts/opencode-code-review.test.sh index f982c951d8..2046127f0b 100644 --- a/scripts/opencode-code-review.test.sh +++ b/scripts/opencode-code-review.test.sh @@ -94,6 +94,35 @@ if [[ "$explicit_model_env_count" != "2" ]]; then fi pass "OpenCode posting steps explicitly pass OPENCODE_MODEL" +run_id_env_count="$(count_occurrences 'GITHUB_RUN_ID: ${{ github.run_id }}')" +run_attempt_env_count="$(count_occurrences 'GITHUB_RUN_ATTEMPT: ${{ github.run_attempt }}')" +if [[ "$run_id_env_count" -ne 2 || "$run_attempt_env_count" -ne 2 ]]; then + fail "Dedicated terminal posting passes immutable workflow run identity" +fi +pass "Dedicated terminal posting passes immutable workflow run identity" + +trusted_environment_count="$(count_occurrences 'environment: opencode-review-trusted')" +if [[ "$trusted_environment_count" -ne 1 ]]; then + fail "Same-repository trusted review uses the protected environment" +fi +if grep -q 'OPENCODE_REVIEW_APP_PRIVATE_KEY' "$WORKFLOW" || ! grep -q 'OPENCODE_REVIEW_ENV_APP_PRIVATE_KEY' "$WORKFLOW"; then + fail "Trusted review uses only the environment-scoped App private key" +fi +pass "Same-repository trusted review isolates App credentials in the protected environment" + +for harness_file in \ + .agents/agents/pr-poller.md \ + .codex/agents/pr-poller.toml \ + .cursor/agents/pr-poller.md \ + .opencode/agents/pr-poller.md \ + .agents/skills/pr-fixup/SKILL.md \ + .agents/skills/planner-orchestration/SKILL.md; do + if ! grep -q 'OpenCode App.*trusted_producer=true\|trusted_producer=true.*OpenCode App\|OpenCode producer.*trusted_producer=true' "$ROOT_DIR/$harness_file"; then + fail "Harness requires trusted producer only for the dedicated OpenCode App" + fi +done +pass "Harness preserves generic selected-reviewer qualification without App provenance" + patch_capability_count="$(count_occurrences 'post-findings --help | grep -q -- "--patch"')" if [[ "$patch_capability_count" != "2" ]]; then fail "OpenCode posting steps check trusted parser support before passing review.patch" @@ -126,3 +155,60 @@ if [[ -n "$invalid_artifact_upload_refs" ]]; then fail "OpenCode review artifact uploads use immutable action refs" fi pass "OpenCode review artifact uploads use immutable action refs" + +if rg -q '^ pull_request:' "$WORKFLOW"; then + fail "OpenCode review runs only from pull_request_target base workflow" +fi +pass "OpenCode review runs only from pull_request_target base workflow" + +app_token_action='actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0' +if [[ "$(count_occurrences "$app_token_action")" != "1" ]]; then + fail "Only same-repository trusted review mints a pinned dedicated App token" +fi +pass "Only same-repository trusted review mints a pinned dedicated App token" + +if [[ "$(count_occurrences 'permission-pull-requests: write')" != "1" ]]; then + fail "Dedicated App tokens explicitly request pull-request write permission" +fi +pass "Dedicated App tokens explicitly request pull-request write permission" + +if [[ "$(count_occurrences 'GH_TOKEN: ${{ steps.app-token.outputs.token }}')" != "1" ]]; then + fail "Only dedicated App tokens are supplied to terminal posting" +fi +pass "Only dedicated App tokens are supplied to terminal posting" + +if [[ "$(count_occurrences 'GH_TOKEN: ${{ github.token }}')" != "1" ]]; then + fail "Only advisory fork posting uses github.token" +fi +pass "Only advisory fork posting uses github.token" + +if [[ "$(count_occurrences 'ref: ${{ github.event.pull_request.head.sha }}')" != "2" ]] || [[ "$(count_occurrences 'git rev-parse HEAD')" != "2" ]]; then + fail "Both review paths use and assert immutable PR head SHA checkouts" +fi +pass "Both review paths use and assert immutable PR head SHA checkouts" + +if [[ "$(count_occurrences 'RANGE="$BASE_SHA...$HEAD_SHA"')" != "2" ]] || rg -q 'incremental|BEFORE|AFTER' "$WORKFLOW"; then + fail "Both review paths always review the full base-to-head PR range" +fi +pass "Both review paths always review the full base-to-head PR range" + +if [[ "$(count_matches regex '^[[:space:]]+pull-requests: read$')" != "1" ]] || [[ "$(count_matches regex '^[[:space:]]+pull-requests: write$')" != "1" ]]; then + fail "Same-repository token is read-only and advisory fork publishing is the only write job" +fi +pass "Same-repository token is read-only and advisory fork publishing is the only write job" + +model_steps="$(awk '/- name: Run OpenCode review/{in_run=1} /- name: Create dedicated OpenCode review App token/{in_run=0} in_run' "$WORKFLOW")" +if rg -q '(OPENCODE_REVIEW_APP_PRIVATE_KEY|steps\.app-token\.outputs\.token)' <<<"$model_steps"; then + fail "OpenCode model step must not receive dedicated App credentials" +fi +pass "OpenCode model step does not receive dedicated App credentials" + +if ! rg -A 10 '^ strip-safe-to-review:' "$WORKFLOW" | rg -q 'issues: write'; then + fail "Fork safe-to-review cleanup has issues write permission" +fi +pass "Fork safe-to-review cleanup has issues write permission" + +if [[ "$(count_occurrences "$app_token_action")" != "1" ]]; then + fail "Only same-repository review mints the dedicated App token" +fi +pass "Only same-repository review mints the dedicated App token" diff --git a/scripts/pr-state b/scripts/pr-state index 02f7cb3fe1..8337d7e729 100755 --- a/scripts/pr-state +++ b/scripts/pr-state @@ -2,7 +2,7 @@ # pr-state — emit a raw JSON snapshot of PR CI and review state. # # Usage: -# scripts/pr-state [--summary] [--all] <PR> +# scripts/pr-state [--summary] [--all] [--trusted-reviewer <login>] <PR> # scripts/pr-state --comment <comment_id> # # Raw output: @@ -10,11 +10,18 @@ # - pr: { number, branch } # - since: { head_oid, committed_at } or null # - checks: [{ type, name, workflow, status, conclusion, details_url, target_url, run_id, job_id }] +# - checks_head_sha: <SHA or null> +# - checks_snapshot_complete: <boolean> # - review_threads: [{ thread_id, is_resolved, path, comment_id, comment_created_at, author, body_preview }] # - hidden_unresolved_threads: [{ thread_id, comment_id, author, path, body_preview }] # - unresolved_review_thread_count: <N> # - filtered_review_thread_count: <N> -# - reviews: [{ author, state, submitted_at, html_url }] +# - reviews: [{ author, state, commit_id, body, submitted_at, html_url }] +# - review_evidence: { known, complete, opening_head_sha, closing_head_sha, +# current_head_sha, evidence_head_sha, exact_current_head_reviews, +# trusted_default_producer_exact_current_head_reviews, +# blocked_exact_current_head_reviews, blocked_exact_current_head_review_count, +# active_changes_requested, active_changes_requested_count } # - issue_comments: [{ author, body, created_at, url }] # - errors: [{ source, message }] # @@ -24,10 +31,17 @@ # - since: { head_oid, committed_at } or null # - failed_checks: [{ name, workflow, status, conclusion, run_id, job_id, details_url, target_url }] # - pending_checks: [{ name, workflow, status, run_id, job_id, details_url, target_url }] +# - checks_head_sha: <SHA or null> +# - checks_snapshot_complete: <boolean> # - unresolved_review_thread_count: <N> # - filtered_review_thread_count: <N> # - unresolved_threads: [{ thread_id, comment_id, author, path, body_preview }] # - hidden_unresolved_threads: [{ thread_id, comment_id, author, path, body_preview }] +# - review_evidence: { known, complete, opening_head_sha, closing_head_sha, +# current_head_sha, evidence_head_sha, exact_current_head_reviews, +# trusted_default_producer_exact_current_head_reviews, +# blocked_exact_current_head_reviews, blocked_exact_current_head_review_count, +# active_changes_requested, active_changes_requested_count } # - summary_notes: [<short human-readable notes>] # - errors: [{ source, message }] # @@ -50,6 +64,7 @@ usage() { INCLUDE_ALL=0 SUMMARY=0 +TRUSTED_REVIEWER='' COMMENT_ID='' PR='' while [[ $# -gt 0 ]]; do @@ -62,6 +77,13 @@ while [[ $# -gt 0 ]]; do SUMMARY=1 shift ;; + --trusted-reviewer) + shift + [[ $# -gt 0 && -z "${TRUSTED_REVIEWER:-}" ]] || usage 1 + TRUSTED_REVIEWER=$1 + [[ -n "$TRUSTED_REVIEWER" && "$TRUSTED_REVIEWER" != -* ]] || usage 1 + shift + ;; --comment) shift [[ $# -gt 0 && -z "${COMMENT_ID:-}" && -z "${PR:-}" ]] || usage 1 @@ -103,9 +125,13 @@ fi errors='[]' pr_json='{}' checks_json='[]' +checks_head_sha='' +checks_snapshot_complete=false review_threads_json='[]' hidden_unresolved_threads_json='[]' reviews_json='[]' +reviews_api_json='[]' +reviews_known=false issue_comments_json='[]' since_json='null' unresolved_review_thread_count=null @@ -177,20 +203,6 @@ extract_checks_json() { ((.state // "") | lower_or_null) as $state | if $state == "pending" or $state == "expected" then null else $state end end; - def preferred_check_order: - if .conclusion == "success" then 0 - elif .status != "completed" then 1 - elif ( - .conclusion != null - and .conclusion != "neutral" - and .conclusion != "skipped" - and .conclusion != "cancelled" - ) then 2 - elif .conclusion == "neutral" then 3 - elif .conclusion == "skipped" then 4 - elif .conclusion == "cancelled" then 5 - else 6 - end; $checks_in[0] | to_entries | map(.value + {_index: .key} | { @@ -209,10 +221,18 @@ extract_checks_json() { | group_by([(.workflow // ""), .name]) | map({ first_index: (map(._index) | min), - items: sort_by(preferred_check_order, ._index) + items: sort_by((.run_id | tonumber? // -1), (.job_id | tonumber? // -1), ._index) }) | sort_by(.first_index) - | map(.items[]) + | map( + .items as $items + | ($items | last) as $latest + | if ($latest.conclusion == "cancelled" or $latest.conclusion == "skipped" or $latest.conclusion == "neutral") then + ([$items[] | select(.conclusion == "failure" or .conclusion == "timed_out" or .conclusion == "error" or .conclusion == "action_required")] | last) // $latest + else + $latest + end + ) | map(del(._index)) ' } @@ -227,7 +247,15 @@ extract_issue_comments_json() { author: (.author.login // "unknown"), body: (.body // ""), created_at: (.createdAt // .created_at // null), - url: (.url // null) + url: (.url // null), + actionable: ( + (((.author.login // "unknown") == "github-actions") or ((.author.login // "unknown") == "github-actions[bot]")) + and ( + (((.body // "") | gsub("^[[:space:]]+|[[:space:]]+$"; "")) + | test("^<!-- opencode-review:no-findings -->\\n\\*\\*OpenCode review complete\\*\\*\\n\\nOpenCode model: `[^`\\n]+`\\n\\nNo suggestions found for commit `[0-9A-Fa-f]{7,40}`\\.$") + | not) + ) + ) }) ' } @@ -241,12 +269,170 @@ extract_reviews_json() { | map({ author: (.user.login // "unknown"), state: ((.state // "unknown") | ascii_downcase), + commit_id: (.commit_id // null), + body: (.body // ""), submitted_at: (.submitted_at // null), html_url: (.html_url // null) }) ' } +review_evidence_json() { + local opening_head_oid=$1 + local closing_head_oid=$2 + local checks_head_oid=$3 + local reviews_known=$4 + local reviews_api_json=$5 + local is_cross_repository=$6 + local trusted_workflow_json=$7 + jq -cn --arg opening_head_oid "$opening_head_oid" --arg closing_head_oid "$closing_head_oid" --arg checks_head_oid "$checks_head_oid" --arg trusted_reviewer "$TRUSTED_REVIEWER" --argjson is_cross_repository "$is_cross_repository" --argjson reviews_known "$reviews_known" --argjson trusted_workflow "$trusted_workflow_json" \ + --slurpfile reviews <(printf '%s\n' "$reviews_api_json") ' + def normalized_reviews: + $reviews[0] | map({ + author: (.user.login // "unknown"), + state: ((.state // "unknown") | ascii_downcase), + commit_id: (.commit_id // null), + body: (.body // ""), + submitted_at: (.submitted_at // null), + html_url: (.html_url // null) + }); + def markdown_prose_lines: + reduce (split("\n")[]) as $line ( + {fence: null, lines: []}; + ($line | (capture("^[[:space:]]*(?<marker>`{3,}|~{3,})")? // {marker: null}) | .marker) as $marker + | if .fence == null then + if $marker == null then .lines += [$line] + else .fence = ($marker[0:1]) + end + elif $marker != null and ($marker[0:1]) == .fence then + .fence = null + else . + end + ) + | .lines; + def has_positive_blocker_label: + markdown_prose_lines + | any(. | gsub("\\*\\*|__|_|\\*"; "") | test("^[[:space:]]*(?:>\\s*)?(?:(?:[-*+]\\s+)|(?:[0-9]+\\.\\s+))?blockers?[[:space:]]*:[[:space:]]*(?!0\\b|none\\b|no\\b)\\S"; "i")); + def has_explicit_blocker_statement: + markdown_prose_lines + | any( + . + | gsub("^[[:space:]]*(?:(?:[-*+]\\s+)|(?:[0-9]+\\.\\s+))?(?:\\*\\*|__|_|\\*)?"; "") + | test("^(?:blocked|changes[ _-]?requested|action required|must fix|required fix|please fix)\\b"; "i") + ); + def review_eligibility: + if ((.body | test("\\|\\s*blocker\\s*\\|\\s*[1-9][0-9]*\\s*\\|"; "i")) or (.body | has_positive_blocker_label) or (.body | has_explicit_blocker_statement)) then {eligibility: "blocked", verdict: "blocker"} + elif .state == "approved" then {eligibility: "eligible", verdict: "approved"} + elif .state == "commented" and (.body | test("<!-- kandev-review: clean -->"; "i")) then {eligibility: "eligible", verdict: "clean"} + elif .state == "commented" then {eligibility: "unknown", verdict: "unknown"} + else {eligibility: "ineligible", verdict: .state} + end; + def trusted_default_producer: + (.body | capture("<!-- kandev-review: workflow-run id=(?<id>[0-9]+) attempt=(?<attempt>[0-9]+) -->")? // null) as $marker + | $marker != null + and $trusted_reviewer != "" + and $is_cross_repository == false + and .author == $trusted_reviewer + and .state == "commented" + and (.body | contains("<!-- kandev-review: clean -->")) + and $trusted_workflow.complete == true + and $marker.id == ($trusted_workflow.id | tostring) + and $marker.attempt == ($trusted_workflow.attempt | tostring); + ($opening_head_oid != "" and $closing_head_oid != "" and $checks_head_oid != "" and $opening_head_oid == $closing_head_oid and $opening_head_oid == $checks_head_oid and $reviews_known) as $complete + | (normalized_reviews) as $all + | (if $complete then [$all[] | select(.commit_id == $opening_head_oid) | . + review_eligibility + {trusted_default_producer: trusted_default_producer}] else [] end) as $exact + | { + known: $complete, + complete: $complete, + opening_head_sha: (if $opening_head_oid == "" then null else $opening_head_oid end), + closing_head_sha: (if $closing_head_oid == "" then null else $closing_head_oid end), + checks_head_sha: (if $checks_head_oid == "" then null else $checks_head_oid end), + current_head_sha: (if $closing_head_oid == "" then null else $closing_head_oid end), + evidence_head_sha: (if $complete then $opening_head_oid else null end), + is_cross_repository: $is_cross_repository, + trusted_workflow_run: $trusted_workflow, + exact_current_head_reviews: $exact, + trusted_default_producer_exact_current_head_reviews: [$exact[] | select(.trusted_default_producer)], + blocked_exact_current_head_reviews: [$exact[] | select(.eligibility == "blocked")], + active_changes_requested: (if $complete then + [$all[] | select(.commit_id == $opening_head_oid)] | group_by(.author) + | map( + map(select( + .state == "approved" + or .state == "changes_requested" + or .state == "dismissed" + or (.state == "commented" and (.body | test("<!-- kandev-review: clean -->"; "i"))) + )) + | sort_by(.submitted_at // "") + | last + ) + | map(select(.state == "changes_requested")) + else [] end) + } + | .active_changes_requested_count = (.active_changes_requested | length) + | .blocked_exact_current_head_review_count = (.blocked_exact_current_head_reviews | length) + | .trusted_producer = (if $complete | not or $trusted_workflow.known != true then "unknown" + elif $trusted_workflow.complete == true and (.trusted_default_producer_exact_current_head_reviews | length) > 0 then "true" + else "false" + end) + ' +} + +fetch_trusted_workflow_json() { + local owner=$1 repo=$2 pr_number=$3 head_sha=$4 + local empty='{"known":false,"complete":false,"id":null,"attempt":null,"status":null,"conclusion":null,"job_complete":false}' + if [[ -z "$TRUSTED_REVIEWER" ]]; then + printf '%s\n' "$empty" + return + fi + if [[ "$is_cross_repository" != "false" || -z "$owner" || -z "$repo" || -z "$pr_number" || -z "$head_sha" ]]; then + add_error "trusted_workflow" "skipped: same-repository PR identity or stable head unavailable" + printf '%s\n' "$empty" + return + fi + local runs_pages runs selected run_id attempt jobs_pages + if ! runs_pages="$(gh_json api --paginate "repos/$owner/$repo/actions/workflows/opencode-code-review.yml/runs?event=pull_request_target&per_page=100")"; then + add_error "trusted_workflow" "workflow runs API failed" + printf '%s\n' "$empty" + return + fi + runs="$(jq -cs '[.[].workflow_runs[]?]' <<<"$runs_pages")" + selected="$(jq -c --argjson pr "$pr_number" --arg head "$head_sha" ' + map(select( + (.id | type == "number") and (.run_number | type == "number") and (.run_attempt | type == "number") + and .path == ".github/workflows/opencode-code-review.yml" + and .event == "pull_request_target" + and ((.pull_requests | type) == "array") + and any(.pull_requests[]?; (.number == $pr) and (.head.sha == $head)) + )) | sort_by(.run_number, .run_attempt, .id) | last // empty + ' <<<"$runs")" + if [[ -z "$selected" ]]; then + add_error "trusted_workflow" "no authentic workflow run matched PR head" + printf '%s\n' "$empty" + return + fi + run_id="$(jq -r '.id' <<<"$selected")" + attempt="$(jq -r '.run_attempt' <<<"$selected")" + if ! jobs_pages="$(gh_json api --paginate "repos/$owner/$repo/actions/runs/$run_id/attempts/$attempt/jobs?per_page=100")"; then + add_error "trusted_workflow" "workflow jobs API failed" + jq -c '. + {known: true, complete: false, attempt: .run_attempt, job_complete: false} | {known, complete, id, attempt, status, conclusion, job_complete}' <<<"$selected" + return + fi + jq -cs --argjson run "$selected" ' + [.[].jobs[]?] as $jobs + | ($jobs | any(.[]; .name == "opencode-review-same-repo" and .status == "completed" and .conclusion == "success")) as $job_complete + | { + known: true, + complete: (($run.status == "completed") and ($run.conclusion == "success") and $job_complete), + id: $run.id, + attempt: $run.run_attempt, + status: ($run.status // null), + conclusion: ($run.conclusion // null), + job_complete: $job_complete + } + ' <<<"$jobs_pages" +} + extract_review_threads_json() { local threads_nodes_json=$1 local since=$2 @@ -300,6 +486,8 @@ summarize_state_json() { { pr, since, + checks_head_sha, + checks_snapshot_complete, failed_checks: [ effective_checks[] | select(terminal_failure_status and bad_conclusion) @@ -312,6 +500,8 @@ summarize_state_json() { ], unresolved_review_thread_count, filtered_review_thread_count, + review_evidence, + actionable_issue_comment_count: ([.issue_comments[] | select(.actionable)] | length), unresolved_threads: [ .review_threads[] | select(.is_resolved == false) @@ -459,16 +649,19 @@ fi comments_json='[]' status_json='[]' resolved_pr_number='' -if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,statusCheckRollup,comments,url)"; then +is_cross_repository=null +if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,headRefOid,isCrossRepository,statusCheckRollup,comments,url)"; then resolved_pr_number="$(jq -r '.number // ""' <<<"$pr_view_json")" if pr_url_owner="$(jq -r '.url | capture("github.com/(?<owner>[^/]+)/(?<repo>[^/]+)/pull/[0-9]+").owner // ""' <<<"$pr_view_json" 2>/dev/null)" && [[ -n "$pr_url_owner" ]]; then repo_owner="$pr_url_owner" repo_name="$(jq -r '.url | capture("github.com/(?<owner>[^/]+)/(?<repo>[^/]+)/pull/[0-9]+").repo // ""' <<<"$pr_view_json")" fi - pr_json="$(jq -c '{number: .number, branch: (.headRefName // "unknown")}' <<<"$pr_view_json")" + pr_json="$(jq -c '{number: .number, branch: (.headRefName // "unknown"), is_cross_repository: (if has("isCrossRepository") then .isCrossRepository else null end)}' <<<"$pr_view_json")" + is_cross_repository="$(jq -c 'if has("isCrossRepository") then .isCrossRepository else null end' <<<"$pr_view_json")" comments_json="$(jq -c '.comments // []' <<<"$pr_view_json")" status_json="$(jq -c '.statusCheckRollup // []' <<<"$pr_view_json")" checks_json="$(extract_checks_json "$status_json")" + checks_head_sha="$(jq -r '.headRefOid // ""' <<<"$pr_view_json")" else if [[ "$PR" =~ ^[0-9]+$ ]]; then pr_json="$(jq -cn --argjson number "$PR" '{number: $number, branch: "unknown"}')" @@ -498,6 +691,7 @@ if [[ -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then if reviews_pages_json="$(gh_json api --paginate -X GET "repos/$repo_owner/$repo_name/pulls/$resolved_pr_number/reviews")"; then reviews_api_json="$(jq -cs 'add // []' <<<"$reviews_pages_json")" reviews_json="$(extract_reviews_json "$reviews_api_json" "$since")" + reviews_known=true else add_error "reviews" "gh api pulls reviews failed" fi @@ -505,6 +699,8 @@ else add_error "reviews" "skipped: resolved PR identity unavailable" fi +opening_head_oid="$(jq -r '.head_oid // ""' <<<"$since_json")" + if [[ -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then if threads_nodes_json="$(fetch_review_threads_nodes_json "$repo_owner" "$repo_name" "$resolved_pr_number")"; then review_threads_json="$(extract_review_threads_json "$threads_nodes_json" "$since")" @@ -529,6 +725,30 @@ elif [[ -z "$resolved_pr_number" ]]; then add_error "review_threads" "skipped: resolved PR identity unavailable" fi +trusted_workflow_json="$(fetch_trusted_workflow_json "$repo_owner" "$repo_name" "$resolved_pr_number" "$opening_head_oid")" +if [[ -n "$TRUSTED_REVIEWER" && "$is_cross_repository" == "false" && "$(jq -r '.known' <<<"$trusted_workflow_json")" != "true" ]]; then + add_error "trusted_workflow" "authenticated workflow evidence unavailable" +fi + +closing_head_oid='' +if [[ -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then + if closing_since_json="$(fetch_head_commit_json "$repo_owner" "$repo_name" "$resolved_pr_number")"; then + closing_head_oid="$(jq -r '.head_oid // ""' <<<"$closing_since_json")" + if [[ -z "$closing_head_oid" ]]; then + add_error "closing_head" "head commit SHA unavailable after evidence collection" + fi + else + add_error "closing_head" "gh api graphql closing head commit failed" + fi +fi +if [[ -n "$opening_head_oid" && -n "$closing_head_oid" && "$opening_head_oid" != "$closing_head_oid" ]]; then + add_error "review_evidence" "head changed during evidence collection" +fi +if [[ -n "$checks_head_sha" && -n "$closing_head_oid" && "$checks_head_sha" == "$closing_head_oid" ]]; then + checks_snapshot_complete=true +fi +review_evidence_json="$(review_evidence_json "$opening_head_oid" "$closing_head_oid" "$checks_head_sha" "$reviews_known" "$reviews_api_json" "$is_cross_repository" "$trusted_workflow_json")" + state_tmp_dir="$(mktemp -d)" cleanup_state_tmp() { rm -rf "$state_tmp_dir" @@ -541,6 +761,7 @@ printf '%s\n' "$checks_json" >"$state_tmp_dir/checks.json" printf '%s\n' "$review_threads_json" >"$state_tmp_dir/review_threads.json" printf '%s\n' "$hidden_unresolved_threads_json" >"$state_tmp_dir/hidden_unresolved_threads.json" printf '%s\n' "$reviews_json" >"$state_tmp_dir/reviews.json" +printf '%s\n' "$review_evidence_json" >"$state_tmp_dir/review_evidence.json" printf '%s\n' "$issue_comments_json" >"$state_tmp_dir/issue_comments.json" printf '%s\n' "$errors" >"$state_tmp_dir/errors.json" @@ -551,7 +772,10 @@ state_json="$(jq -cn \ --slurpfile review_threads "$state_tmp_dir/review_threads.json" \ --slurpfile hidden_unresolved_threads "$state_tmp_dir/hidden_unresolved_threads.json" \ --slurpfile reviews "$state_tmp_dir/reviews.json" \ + --slurpfile review_evidence "$state_tmp_dir/review_evidence.json" \ --slurpfile issue_comments "$state_tmp_dir/issue_comments.json" \ + --arg checks_head_sha "$checks_head_sha" \ + --argjson checks_snapshot_complete "$checks_snapshot_complete" \ --argjson unresolved_review_thread_count "$unresolved_review_thread_count" \ --argjson filtered_review_thread_count "$filtered_review_thread_count" \ --slurpfile errors "$state_tmp_dir/errors.json" \ @@ -559,12 +783,16 @@ state_json="$(jq -cn \ pr: $pr[0], since: $since[0], checks: $checks[0], + checks_head_sha: (if $checks_head_sha == "" then null else $checks_head_sha end), + checks_snapshot_complete: $checks_snapshot_complete, review_threads: $review_threads[0], hidden_unresolved_threads: $hidden_unresolved_threads[0], unresolved_review_thread_count: $unresolved_review_thread_count, filtered_review_thread_count: $filtered_review_thread_count, reviews: $reviews[0], + review_evidence: $review_evidence[0], issue_comments: $issue_comments[0], + actionable_issue_comment_count: ([$issue_comments[0][] | select(.actionable)] | length), errors: $errors[0] }')" diff --git a/scripts/pr-state.test.sh b/scripts/pr-state.test.sh index e0a60dfe97..c542ddb933 100644 --- a/scripts/pr-state.test.sh +++ b/scripts/pr-state.test.sh @@ -92,6 +92,35 @@ JSON exit 0 fi +if [[ "$*" == *"actions/workflows/opencode-code-review.yml/runs"* ]]; then + if [[ "${GH_FAIL_TRUSTED_WORKFLOW:-0}" == "1" ]]; then + echo "workflow api failed" >&2 + exit 1 + fi + if [[ "${GH_HEAD_SEQUENCE:-stable}" == "after-workflow" ]]; then + : >"${GH_HEAD_COUNTER_FILE:?}" + fi + if [[ "${GH_TRUSTED_MULTI:-0}" == "1" ]]; then + cat <<'JSON' +{"workflow_runs":[{"id":901,"path":".github/workflows/opencode-code-review.yml","event":"pull_request_target","status":"completed","conclusion":"failure","run_number":11,"run_attempt":1,"pull_requests":[{"number":123,"head":{"sha":"abc123"}}]},{"id":900,"path":".github/workflows/opencode-code-review.yml","event":"pull_request_target","status":"completed","conclusion":"success","run_number":10,"run_attempt":2,"pull_requests":[{"number":123,"head":{"sha":"abc123"}}]}]} +JSON + exit 0 + fi + conclusion="${GH_TRUSTED_WORKFLOW_CONCLUSION:-success}" + status="${GH_TRUSTED_WORKFLOW_STATUS:-completed}" + cat <<JSON +{"workflow_runs":[{"id":900,"path":".github/workflows/opencode-code-review.yml","event":"pull_request_target","status":"$status","conclusion":"$conclusion","run_number":10,"run_attempt":2,"pull_requests":[{"number":123,"head":{"sha":"abc123"}}]}]} +JSON + exit 0 +fi + +if [[ "$*" == *"actions/runs/900/attempts/2/jobs"* ]]; then + cat <<'JSON' +{"jobs":[{"name":"opencode-review-same-repo","status":"completed","conclusion":"success"}]} +JSON + exit 0 +fi + if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then if [[ "${GH_NO_PR_URL:-0}" == "1" ]]; then pr_url='null' @@ -102,6 +131,8 @@ if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then { "number": 123, "headRefName": "feat/pr-state", + "headRefOid": "${GH_CHECKS_HEAD:-abc123}", + "isCrossRepository": ${GH_CROSS_REPOSITORY:-false}, "url": $pr_url, "comments": [ { @@ -114,6 +145,37 @@ if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then "body": "**Claude finished review**\\n| Blocker | 1 |\\n| Suggestion | 2 |\\n**Verdict:** Ready with suggestions", "createdAt": "2026-06-01T13:00:00Z", "url": "https://github.com/kdlbs/kandev/pull/123#issuecomment-2" + }, + { + "author": { "login": "github-actions" }, + "body": "<!-- opencode-review:no-findings -->\\n**OpenCode review complete**\\n\\nOpenCode model: \`opencode-go/minimax-m3\`\\n\\nNo suggestions found for commit \`abc1234\`.", + "createdAt": "2026-06-01T13:01:00Z", + "url": "https://github.com/kdlbs/kandev/pull/123#issuecomment-3" + }, + { + "author": { "login": "other-bot[bot]" }, + "body": "<!-- opencode-review:no-findings -->\\n**OpenCode review complete**\\n\\nOpenCode model: \`opencode-go/minimax-m3\`\\n\\nNo suggestions found for commit \`abc1234\`.", + "createdAt": "2026-06-01T13:02:00Z" + }, + { + "author": { "login": "github-actions" }, + "body": "Review quotes <!-- opencode-review:no-findings --> later", + "createdAt": "2026-06-01T13:03:00Z" + }, + { + "author": { "login": "github-actions" }, + "body": "<!-- opencode-review:no-findings -->\\n**OpenCode review complete**\\nBlocker: real issue", + "createdAt": "2026-06-01T13:04:00Z" + }, + { + "author": { "login": "github-actions" }, + "body": "<!-- opencode-review:diagnostic --> failure", + "createdAt": "2026-06-01T13:05:00Z" + }, + { + "author": { "login": "github-actions" }, + "body": "<!-- opencode-review:fallback-findings --> finding", + "createdAt": "2026-06-01T13:06:00Z" } ], "statusCheckRollup": [ @@ -184,6 +246,22 @@ if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then "conclusion": "FAILURE", "detailsUrl": "https://github.com/kdlbs/kandev/actions/runs/27340000007/job/55150000007" }, + { + "__typename": "CheckRun", + "name": "late-cancelled-failure", + "workflowName": "CI", + "status": "COMPLETED", + "conclusion": "FAILURE", + "detailsUrl": "https://github.com/kdlbs/kandev/actions/runs/27340000008/job/55150000008" + }, + { + "__typename": "CheckRun", + "name": "late-cancelled-failure", + "workflowName": "CI", + "status": "COMPLETED", + "conclusion": "CANCELLED", + "detailsUrl": "https://github.com/kdlbs/kandev/actions/runs/27340000009/job/55150000009" + }, { "__typename": "StatusContext", "context": "external pending", @@ -197,10 +275,198 @@ JSON fi if [[ "$1" == "api" && "$2" == "--paginate" && "$3" == "-X" && "$4" == "GET" && "$5" == "repos/kdlbs/kandev/pulls/123/reviews" ]]; then + if [[ "${GH_REVIEW_SCENARIO:-default}" == "old-head" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"APPROVED","commit_id":"old-head-sha","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "exact-head" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"APPROVED","commit_id":"abc123","body":"Reviewed and approved.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "github-actions-clean" ]]; then + printf '%s\n' '[ + {"user":{"login":"github-actions[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\\n**OpenCode review complete**","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "dedicated-app-clean" ]]; then + printf '%s\n' '[ + {"user":{"login":"opencode-review-app[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\\n<!-- kandev-review: workflow-run id=900 attempt=2 -->\\n**OpenCode review complete**","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "dedicated-app-wrong-run" ]]; then + printf '%s\n' '[ + {"user":{"login":"opencode-review-app[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\\n<!-- kandev-review: workflow-run id=899 attempt=1 -->","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "github-actions-old-head" ]]; then + printf '%s\n' '[ + {"user":{"login":"github-actions[bot]"},"state":"COMMENTED","commit_id":"old-head-sha","body":"<!-- kandev-review: clean -->\\n**OpenCode review complete**","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "exact-dismissed" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"DISMISSED","commit_id":"abc123","body":"Dismissed after update.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "approved-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"APPROVED","commit_id":"abc123","body":"Blocker: the approval does not waive this required fix.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "comment-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blocker: please fix the race before merge.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "plain-selected-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blocker: race condition in the retry loop.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "plain-nonselected-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo blockers.","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"No blockers in unrelated files.\nBlockers: race condition in this path.","submitted_at":"2026-06-01T13:31:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "negated-colon-labels" ]]; then + printf '%s\n' '[ + {"user":{"login":"one[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blocker: 0","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"two[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blockers: 0","submitted_at":"2026-06-01T13:31:00Z"}, + {"user":{"login":"three[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blocker: none","submitted_at":"2026-06-01T13:32:00Z"}, + {"user":{"login":"four[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Blockers: no issues","submitted_at":"2026-06-01T13:33:00Z"}, + {"user":{"login":"five[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"No blocker: all clear","submitted_at":"2026-06-01T13:34:00Z"}, + {"user":{"login":"six[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"No blockers: all clear","submitted_at":"2026-06-01T13:35:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "negated-then-positive-label" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"No blockers: all clear\nBlocker: race condition","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "formatted-negative-labels" ]]; then + printf '%s\n' '[ + {"user":{"login":"one[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"**Blocker:** none","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"two[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"> **Blockers:** 0","submitted_at":"2026-06-01T13:31:00Z"}, + {"user":{"login":"three[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"_Blocker:_ no issue","submitted_at":"2026-06-01T13:32:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "formatted-positive-labels" ]]; then + printf '%s\n' '[ + {"user":{"login":"one[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"**Blocker:** race condition","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"two[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"> **Blockers:** retry race","submitted_at":"2026-06-01T13:31:00Z"}, + {"user":{"login":"three[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"- _Blocker:_ missing lock","submitted_at":"2026-06-01T13:32:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "single-asterisk-labels" ]]; then + printf '%s\n' '[ + {"user":{"login":"one[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"*Blocker:* race condition","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"two[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"*Blocker:* none","submitted_at":"2026-06-01T13:31:00Z"}, + {"user":{"login":"three[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"*Blockers:* 0","submitted_at":"2026-06-01T13:32:00Z"}, + {"user":{"login":"four[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"* Blocker: race condition","submitted_at":"2026-06-01T13:33:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "comment-clean" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo findings.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "comment-zero-table" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\n| Blocker | 0 |\nNo findings.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "comment-no-blockers" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo blockers. No findings.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "comment-positive-table" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\n| Blocker | 2 |\nMust fix before merge.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "clean-plus-current-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo findings.","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"Must fix this race before merge.","submitted_at":"2026-06-01T13:31:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "clean-plus-old-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo findings.","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"COMMENTED","commit_id":"old-head-sha","body":"Must fix this old issue before merge.","submitted_at":"2026-06-01T13:31:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "changes-dismissed" ]]; then + printf '%s\n' '[ + {"user":{"login":"cubic-dev-ai[bot]"},"state":"CHANGES_REQUESTED","commit_id":"abc123","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"DISMISSED","commit_id":"abc123","submitted_at":"2026-06-01T13:45:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "changes-current-clean" ]]; then + printf '%s\n' '[ + {"user":{"login":"cubic-dev-ai[bot]"},"state":"CHANGES_REQUESTED","commit_id":"abc123","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo findings.","submitted_at":"2026-06-01T13:45:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "old-change-current-clean" ]]; then + printf '%s\n' '[ + {"user":{"login":"cubic-dev-ai[bot]"},"state":"CHANGES_REQUESTED","commit_id":"old-head-sha","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\\nNo findings.","submitted_at":"2026-06-01T13:45:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "current-change-old-approval" ]]; then + printf '%s\n' '[ + {"user":{"login":"cubic-dev-ai[bot]"},"state":"CHANGES_REQUESTED","commit_id":"abc123","submitted_at":"2026-06-01T13:30:00Z"}, + {"user":{"login":"cubic-dev-ai[bot]"},"state":"APPROVED","commit_id":"old-head-sha","submitted_at":"2026-06-01T13:45:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "quoted-blocker" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\\nPrior review quoted: please fix the race before merge.","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi + if [[ "${GH_REVIEW_SCENARIO:-default}" == "fenced-blocker-example" ]]; then + printf '%s\n' '[ + {"user":{"login":"greptile-apps[bot]"},"state":"COMMENTED","commit_id":"abc123","body":"<!-- kandev-review: clean -->\nNo findings.\n\n```text\nplease fix the example\nBlocker: illustrative only\n```","submitted_at":"2026-06-01T13:30:00Z"} + ]' + exit 0 + fi printf '%s\n' '[ { "user": { "login": "greptile-apps[bot]" }, "state": "COMMENTED", + "commit_id": "old-head-sha", "submitted_at": "2026-06-01T10:30:00Z" } ]' @@ -208,24 +474,35 @@ if [[ "$1" == "api" && "$2" == "--paginate" && "$3" == "-X" && "$4" == "GET" && { "user": { "login": "cubic-dev-ai[bot]" }, "state": "CHANGES_REQUESTED", + "commit_id": "abc123", "submitted_at": "2026-06-01T13:30:00Z" } ]' exit 0 fi -if [[ "$1" == "api" && "$2" == "graphql" ]]; then + if [[ "$1" == "api" && "$2" == "graphql" ]]; then if [[ "$*" == *"headRefOid"* ]]; then + head_oid="abc123" + if [[ "${GH_HEAD_SEQUENCE:-stable}" == "race" ]]; then + if [[ -f "${GH_HEAD_COUNTER_FILE:?}" ]]; then + head_oid="def456" + else + : >"$GH_HEAD_COUNTER_FILE" + fi + elif [[ "${GH_HEAD_SEQUENCE:-stable}" == "after-workflow" && -f "${GH_HEAD_COUNTER_FILE:?}" ]]; then + head_oid="def456" + fi printf '%s\n' '{ "data": { "repository": { "pullRequest": { - "headRefOid": "abc123", + "headRefOid": "'"$head_oid"'", "commits": { "nodes": [ { "commit": { - "oid": "abc123", + "oid": "'"$head_oid"'", "committedDate": "2026-06-01T12:00:00Z" } } @@ -381,12 +658,13 @@ test_snapshot_happy_path() { assert_jq "pr number" '.pr.number == 123' "$json" assert_jq "branch" '.pr.branch == "feat/pr-state"' "$json" assert_jq "since timestamp" '.since.committed_at == "2026-06-01T12:00:00Z"' "$json" - assert_jq "checks count" '.checks | length == 9' "$json" - assert_jq "duplicate cancelled check prefers success first" '[.checks[] | select(.name == "web lint")][0] | .conclusion == "success" and .run_id == "27340000001"' "$json" + assert_jq "checks collapse duplicate workflow attempts" '.checks | length < 10' "$json" + assert_jq "latest duplicate check uses newest attempt" '[.checks[] | select(.name == "web lint")][0] | .conclusion == "success" and .run_id == "27340000001"' "$json" assert_jq "failed check preserved" '.checks[] | select(.name == "e2e") | .conclusion == "failure"' "$json" assert_jq "check run id" '.checks[] | select(.name == "e2e") | .run_id == "27340000002"' "$json" assert_jq "nested workflow name" '.checks[] | select(.name == "e2e") | .workflow == "CI"' "$json" assert_jq "duplicate check prefers non-skipped first" '[.checks[] | select(.name == "opencode-review-same-repo")][0] | .conclusion == "success" and .run_id == "27340000005"' "$json" + assert_jq "duplicate cancellation retains prior failure" '[.checks[] | select(.name == "late-cancelled-failure")][0] | .conclusion == "failure" and .run_id == "27340000008"' "$json" assert_jq "pending status context conclusion normalized" '.checks[] | select(.name == "external pending") | .status == "pending" and .conclusion == null' "$json" assert_jq "threads count" '.review_threads | length == 1' "$json" assert_jq "total unresolved count includes historical unresolved thread" '.unresolved_review_thread_count == 2' "$json" @@ -395,12 +673,369 @@ test_snapshot_happy_path() { assert_jq "thread comment timestamp" '.review_threads[] | select(.thread_id == "PRRT_1") | .comment_created_at == "2026-06-01T13:00:00Z"' "$json" assert_jq "reviews count" '.reviews | length == 1' "$json" assert_jq "review author" '.reviews[] | select(.author == "cubic-dev-ai[bot]") | .author == "cubic-dev-ai[bot]"' "$json" - assert_jq "issue comments count" '.issue_comments | length == 1' "$json" - assert_jq "issue comment author" '.issue_comments[] | select(.author == "github-actions") | .body | contains("Verdict")' "$json" + assert_jq "issue comments count" '.issue_comments | length == 7' "$json" + assert_jq "issue comment author" 'any(.issue_comments[]; .author == "github-actions" and (.body | contains("Verdict")))' "$json" + assert_jq "only GitHub Actions noncanonical output is actionable" '([.issue_comments[] | select(.actionable == false)] | length) == 2 and .actionable_issue_comment_count == 5' "$json" + assert_jq "non-GitHub-Actions canonical marker is nonactionable while workflow diagnostic and fallback remain actionable" 'any(.issue_comments[]; .author == "other-bot[bot]" and .actionable == false) and all(.issue_comments[] | select(.body | test("Blocker: real|diagnostic|fallback"; "i")); .actionable == true)' "$json" assert_jq "no errors" '.errors == []' "$json" + assert_jq "raw review evidence is complete" '.review_evidence.complete == true and .review_evidence.current_head_sha == "abc123"' "$json" + assert_jq "active change request surfaced" '.review_evidence.active_changes_requested_count == 1' "$json" pass "snapshot happy path" } +test_old_head_review_does_not_qualify() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=old-head PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "old head review evidence is complete but not exact" '.review_evidence.complete == true and .review_evidence.exact_current_head_reviews == []' "$json" + pass "old-head review does not qualify" +} + +test_exact_head_selected_review_qualifies() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=exact-head PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "summary exact selected review evidence" '.review_evidence.complete == true and .review_evidence.exact_current_head_reviews[0].author == "greptile-apps[bot]" and .review_evidence.exact_current_head_reviews[0].body == "Reviewed and approved." and .review_evidence.exact_current_head_reviews[0].eligibility == "eligible"' "$json" + pass "exact-head selected review qualifies" +} + +test_clean_marker_requires_configured_dedicated_app_provenance() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local exact_json + GH_REVIEW_SCENARIO=github-actions-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/unconfigured.json" + local unconfigured_json + unconfigured_json="$(<"$tmp/unconfigured.json")" + assert_jq "unconfigured marker is not trusted evidence" '.review_evidence.exact_current_head_reviews[0].trusted_default_producer == false and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$unconfigured_json" + + GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/exact.json" + exact_json="$(<"$tmp/exact.json")" + assert_jq "matching dedicated App has authenticated OpenCode provenance" '.review_evidence.exact_current_head_reviews[0].author == "opencode-review-app[bot]" and .review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.exact_current_head_reviews[0].trusted_default_producer == true and .review_evidence.trusted_workflow_run.id == 900 and .review_evidence.trusted_workflow_run.complete == true and .review_evidence.trusted_producer == "true"' "$exact_json" + + local old_json + GH_REVIEW_SCENARIO=github-actions-old-head PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'github-actions[bot]' 123 >"$tmp/old.json" + old_json="$(<"$tmp/old.json")" + assert_jq "old head actions review never becomes trusted evidence" '.review_evidence.exact_current_head_reviews == [] and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$old_json" + pass "clean marker requires configured dedicated app provenance" +} + +test_trusted_workflow_requires_latest_success_and_matching_marker() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + for conclusion in failure neutral cancelled skipped; do + GH_TRUSTED_WORKFLOW_CONCLUSION="$conclusion" GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/$conclusion.json" + json="$(<"$tmp/$conclusion.json")" + assert_jq "latest $conclusion workflow invalidates old clean review" '.review_evidence.trusted_workflow_run.complete == false and .review_evidence.trusted_producer == "false" and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + done + + GH_TRUSTED_WORKFLOW_STATUS=in_progress GH_TRUSTED_WORKFLOW_CONCLUSION=null GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/pending.json" + json="$(<"$tmp/pending.json")" + assert_jq "latest pending workflow invalidates old clean review" '.review_evidence.trusted_workflow_run.complete == false and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + + GH_REVIEW_SCENARIO=dedicated-app-wrong-run PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/wrong-marker.json" + json="$(<"$tmp/wrong-marker.json")" + assert_jq "wrong workflow marker is untrusted" '.review_evidence.trusted_workflow_run.complete == true and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + + GH_TRUSTED_MULTI=1 GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/reversed-generations.json" + json="$(<"$tmp/reversed-generations.json")" + assert_jq "newer authentic failure beats older success despite reverse API order" '.review_evidence.trusted_workflow_run.id == 901 and .review_evidence.trusted_workflow_run.complete == false and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + + GH_FAIL_TRUSTED_WORKFLOW=1 GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/api-failure.json" + json="$(<"$tmp/api-failure.json")" + assert_jq "workflow API failure is unknown and untrusted" '.review_evidence.trusted_workflow_run.known == false and .review_evidence.trusted_producer == "unknown" and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + pass "trusted workflow requires latest success and matching marker" +} + +test_fork_and_unknown_provenance_are_untrusted() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local fork_json unknown_json + GH_CROSS_REPOSITORY=true GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/fork.json" + fork_json="$(<"$tmp/fork.json")" + assert_jq "fork App marker is untrusted" '.pr.is_cross_repository == true and .review_evidence.is_cross_repository == true and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$fork_json" + + GH_CROSS_REPOSITORY=null GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/unknown.json" + unknown_json="$(<"$tmp/unknown.json")" + assert_jq "unknown repository provenance is untrusted" '.pr.is_cross_repository == null and .review_evidence.is_cross_repository == null and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$unknown_json" + pass "fork and unknown provenance are untrusted" +} + +test_head_change_during_collection_invalidates_review_evidence() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_HEAD_SEQUENCE=race GH_HEAD_COUNTER_FILE="$tmp/head-calls" GH_REVIEW_SCENARIO=exact-head PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "head race is incomplete and cannot qualify" '.review_evidence.known == false and .review_evidence.complete == false and .review_evidence.opening_head_sha == "abc123" and .review_evidence.closing_head_sha == "def456" and .review_evidence.current_head_sha == "def456" and .review_evidence.evidence_head_sha == null and .review_evidence.exact_current_head_reviews == []' "$json" + pass "head change during collection invalidates review evidence" +} + +test_head_change_during_authenticated_workflow_collection_invalidates_trust() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_HEAD_SEQUENCE=after-workflow GH_HEAD_COUNTER_FILE="$tmp/workflow-called" GH_REVIEW_SCENARIO=dedicated-app-clean PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --trusted-reviewer 'opencode-review-app[bot]' 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "post-workflow head race invalidates authenticated trust" '.review_evidence.complete == false and .review_evidence.opening_head_sha == "abc123" and .review_evidence.closing_head_sha == "def456" and .review_evidence.trusted_default_producer_exact_current_head_reviews == []' "$json" + pass "head change during authenticated workflow collection invalidates trust" +} + +test_checks_head_mismatch_invalidates_combined_evidence() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_CHECKS_HEAD=old-checks-sha GH_REVIEW_SCENARIO=exact-head PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "checks and reviews cannot be paired across heads" '.checks_head_sha == "old-checks-sha" and .checks_snapshot_complete == false and .review_evidence.known == false and .review_evidence.complete == false and .review_evidence.checks_head_sha == "old-checks-sha" and .review_evidence.exact_current_head_reviews == []' "$json" + pass "checks head mismatch invalidates combined evidence" +} + +test_exact_head_dismissed_review_is_not_eligible() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=exact-dismissed PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "dismissed exact review is ineligible" '.review_evidence.exact_current_head_reviews[0].state == "dismissed" and .review_evidence.exact_current_head_reviews[0].eligibility == "ineligible"' "$json" + pass "exact-head dismissed review is not eligible" +} + +test_approved_review_with_blocker_is_not_eligible() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=approved-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "approved review body blocker wins" '.review_evidence.exact_current_head_reviews[0].state == "approved" and .review_evidence.exact_current_head_reviews[0].eligibility == "blocked" and .review_evidence.exact_current_head_reviews[0].verdict == "blocker"' "$json" + pass "approved review with blocker is not eligible" +} + +test_commented_blocker_is_not_eligible() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=comment-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "commented blocker is blocked" '(.review_evidence.exact_current_head_reviews[0].body | contains("Blocker")) and .review_evidence.exact_current_head_reviews[0].eligibility == "blocked" and .review_evidence.exact_current_head_reviews[0].verdict == "blocker"' "$json" + pass "commented blocker is not eligible" +} + +test_plain_blocker_labels_are_blocked_and_aggregated() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local selected_json + GH_REVIEW_SCENARIO=plain-selected-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/selected.json" + selected_json="$(<"$tmp/selected.json")" + assert_jq "plain selected blocker is blocked" '.review_evidence.exact_current_head_reviews[0].eligibility == "blocked" and .review_evidence.blocked_exact_current_head_review_count == 1' "$selected_json" + + local nonselected_json + GH_REVIEW_SCENARIO=plain-nonselected-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/nonselected.json" + nonselected_json="$(<"$tmp/nonselected.json")" + assert_jq "plain nonselected blocker aggregates despite no blockers text" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.blocked_exact_current_head_review_count == 1 and .review_evidence.blocked_exact_current_head_reviews[0].author == "cubic-dev-ai[bot]"' "$nonselected_json" + pass "plain blocker labels are blocked and aggregated" +} + +test_colon_blocker_labels_are_line_anchored_and_scan_all_lines() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local negative_json + GH_REVIEW_SCENARIO=negated-colon-labels PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/negative.json" + negative_json="$(<"$tmp/negative.json")" + assert_jq "zero and negated colon labels are nonblocking" '.review_evidence.blocked_exact_current_head_review_count == 0 and ([.review_evidence.exact_current_head_reviews[].eligibility] | all(. != "blocked"))' "$negative_json" + + local mixed_json + GH_REVIEW_SCENARIO=negated-then-positive-label PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/mixed.json" + mixed_json="$(<"$tmp/mixed.json")" + assert_jq "later positive label wins" '.review_evidence.blocked_exact_current_head_review_count == 1 and .review_evidence.exact_current_head_reviews[0].eligibility == "blocked"' "$mixed_json" + pass "colon blocker labels are line anchored and scan all lines" +} + +test_formatted_colon_labels_handle_emphasis_after_colon() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local negative_json + GH_REVIEW_SCENARIO=formatted-negative-labels PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/negative.json" + negative_json="$(<"$tmp/negative.json")" + assert_jq "formatted negative labels are nonblocking" '.review_evidence.blocked_exact_current_head_review_count == 0 and ([.review_evidence.exact_current_head_reviews[].eligibility] | all(. != "blocked"))' "$negative_json" + + local positive_json + GH_REVIEW_SCENARIO=formatted-positive-labels PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/positive.json" + positive_json="$(<"$tmp/positive.json")" + assert_jq "formatted positive labels block" '.review_evidence.blocked_exact_current_head_review_count == 3 and ([.review_evidence.exact_current_head_reviews[].eligibility] | all(. == "blocked"))' "$positive_json" + pass "formatted colon labels handle emphasis after colon" +} + +test_single_asterisk_emphasis_and_list_markers_are_safe() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=single-asterisk-labels PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + assert_jq "single asterisk forms classify correctly" '.review_evidence.blocked_exact_current_head_review_count == 2 and .review_evidence.exact_current_head_reviews[0].eligibility == "blocked" and .review_evidence.exact_current_head_reviews[1].eligibility != "blocked" and .review_evidence.exact_current_head_reviews[2].eligibility != "blocked" and .review_evidence.exact_current_head_reviews[3].eligibility == "blocked"' "$json" + pass "single asterisk emphasis and list markers are safe" +} + +test_explicit_clean_commented_review_is_eligible() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=comment-clean PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "explicit clean comment is eligible" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.exact_current_head_reviews[0].verdict == "clean"' "$json" + pass "explicit clean commented review is eligible" +} + +test_zero_blocker_summaries_remain_clean_with_explicit_marker() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local table_json + GH_REVIEW_SCENARIO=comment-zero-table PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/table.json" + table_json="$(<"$tmp/table.json")" + assert_jq "zero blocker table is clean" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.exact_current_head_reviews[0].verdict == "clean"' "$table_json" + + local no_blockers_json + GH_REVIEW_SCENARIO=comment-no-blockers PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/no-blockers.json" + no_blockers_json="$(<"$tmp/no-blockers.json")" + assert_jq "no blockers text is clean" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.exact_current_head_reviews[0].verdict == "clean"' "$no_blockers_json" + pass "zero blocker summaries remain clean with explicit marker" +} + +test_positive_blocker_evidence_overrides_clean_marker() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=comment-positive-table PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "positive blocker overrides clean marker" '.review_evidence.exact_current_head_reviews[0].eligibility == "blocked" and .review_evidence.exact_current_head_reviews[0].verdict == "blocker"' "$json" + pass "positive blocker evidence overrides clean marker" +} + +test_exact_head_blockers_from_any_author_are_aggregated() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local current_json + GH_REVIEW_SCENARIO=clean-plus-current-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/current.json" + current_json="$(<"$tmp/current.json")" + assert_jq "other author's current blocker is aggregated" '.review_evidence.blocked_exact_current_head_review_count == 1 and .review_evidence.blocked_exact_current_head_reviews[0].author == "cubic-dev-ai[bot]" and .review_evidence.blocked_exact_current_head_reviews[0].eligibility == "blocked"' "$current_json" + + local old_json + GH_REVIEW_SCENARIO=clean-plus-old-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/old.json" + old_json="$(<"$tmp/old.json")" + assert_jq "old-head blocker is excluded" '.review_evidence.blocked_exact_current_head_review_count == 0 and .review_evidence.exact_current_head_reviews[0].eligibility == "eligible"' "$old_json" + pass "exact-head blockers from any author are aggregated" +} + +test_latest_decisive_review_state_controls_active_change_requests() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local blocked_json + PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/blocked.json" + blocked_json="$(<"$tmp/blocked.json")" + assert_jq "latest change request blocks" '.review_evidence.active_changes_requested_count == 1 and .review_evidence.active_changes_requested[0].author == "cubic-dev-ai[bot]"' "$blocked_json" + + local cleared_json + GH_REVIEW_SCENARIO=changes-dismissed PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/cleared.json" + cleared_json="$(<"$tmp/cleared.json")" + assert_jq "later decisive dismissal clears change request" '.review_evidence.active_changes_requested_count == 0' "$cleared_json" + + local clean_json + GH_REVIEW_SCENARIO=changes-current-clean PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/clean.json" + clean_json="$(<"$tmp/clean.json")" + assert_jq "later clean current-head review clears change request" '.review_evidence.active_changes_requested_count == 0' "$clean_json" + + local old_change_current_clean_json + GH_REVIEW_SCENARIO=old-change-current-clean PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/old-change-current-clean.json" + old_change_current_clean_json="$(<"$tmp/old-change-current-clean.json")" + assert_jq "old-head change request does not block a clean current head" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.active_changes_requested_count == 0' "$old_change_current_clean_json" + + local current_change_old_approval_json + GH_REVIEW_SCENARIO=current-change-old-approval PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/current-change-old-approval.json" + current_change_old_approval_json="$(<"$tmp/current-change-old-approval.json")" + assert_jq "old-head approval cannot clear a current-head change request" '.review_evidence.active_changes_requested_count == 1 and .review_evidence.active_changes_requested[0].commit_id == "abc123"' "$current_change_old_approval_json" + pass "latest decisive review state controls active change requests" +} + +test_quoted_blocker_language_does_not_block_clean_review() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=quoted-blocker PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "quoted blocker text preserves explicit clean review" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.blocked_exact_current_head_review_count == 0' "$json" + pass "quoted blocker language does not block clean review" +} + +test_fenced_blocker_examples_do_not_block_clean_review() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_REVIEW_SCENARIO=fenced-blocker-example PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "fenced blocker examples preserve explicit clean review" '.review_evidence.exact_current_head_reviews[0].eligibility == "eligible" and .review_evidence.blocked_exact_current_head_review_count == 0' "$json" + pass "fenced blocker examples do not block clean review" +} + test_partial_failure_records_error_but_keeps_other_data() { local tmp make_tmp_dir tmp @@ -411,8 +1046,8 @@ test_partial_failure_records_error_but_keeps_other_data() { json="$(<"$tmp/out.json")" assert_jq "reviews empty on failure" '.reviews == []' "$json" - assert_jq "checks still present" '.checks | length == 9' "$json" - assert_jq "new issue comments still present" '.issue_comments | length == 1' "$json" + assert_jq "checks still present" '.checks | length < 9' "$json" + assert_jq "new issue comments still present" '.issue_comments | length == 7' "$json" assert_jq "partial failure recorded" '.errors | length == 1' "$json" assert_jq "partial failure source" '.errors[0].source == "reviews"' "$json" pass "partial failure records error but keeps other data" @@ -442,7 +1077,7 @@ test_repo_failure_skips_review_threads() { GH_FAIL_REPO=1 GH_NO_PR_URL=1 PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" json="$(<"$tmp/out.json")" - assert_jq "checks still present on repo failure" '.checks | length == 9' "$json" + assert_jq "checks still present on repo failure" '.checks | length < 9' "$json" assert_jq "review threads empty on repo failure" '.review_threads == []' "$json" assert_jq "unresolved count unknown on repo failure" '.unresolved_review_thread_count == null' "$json" assert_jq "repo failure recorded" '.errors[] | select(.source == "repo") | .message == "gh repo view failed"' "$json" @@ -459,14 +1094,15 @@ test_graphql_failure_records_error_but_keeps_other_data() { GH_FAIL_GRAPHQL=1 PATH="$tmp/bin:$PATH" "$SCRIPT" 123 >"$tmp/out.json" json="$(<"$tmp/out.json")" - assert_jq "checks still present on graphql failure" '.checks | length == 9' "$json" + assert_jq "checks still present on graphql failure" '.checks | length < 9' "$json" assert_jq "reviews still present on graphql failure" '.reviews | length == 2' "$json" assert_jq "review threads empty on graphql failure" '.review_threads == []' "$json" assert_jq "unresolved count unknown on graphql failure" '.unresolved_review_thread_count == null' "$json" - assert_jq "graphql failure records both errors" '.errors | length == 2' "$json" + assert_jq "graphql failure records opening and closing head errors" '.errors | length == 3' "$json" assert_jq "graphql failure records since error" '.errors[] | select(.source == "since") | .message == "gh api graphql head commit failed; including historical comments"' "$json" assert_jq "graphql failure records review_threads error" '.errors[] | select(.source == "review_threads") | .message == "gh api graphql reviewThreads failed"' "$json" - assert_jq "since fallback includes all historical comments" '.issue_comments | length == 2' "$json" + assert_jq "graphql failure records closing head error" '.errors[] | select(.source == "closing_head") | .message == "gh api graphql closing head commit failed"' "$json" + assert_jq "since fallback includes all historical comments" '.issue_comments | length == 8' "$json" pass "graphql failure records error but keeps other data" } @@ -498,7 +1134,7 @@ test_all_flag_includes_historical_comments_and_reviews() { json="$(<"$tmp/out.json")" assert_jq "since omitted in all mode" '.since == null' "$json" - assert_jq "all issue comments present" '.issue_comments | length == 2' "$json" + assert_jq "all issue comments present" '.issue_comments | length == 8' "$json" assert_jq "all reviews present" '.reviews | length == 2' "$json" assert_jq "all review threads present" '.review_threads | length == 2' "$json" assert_jq "all mode keeps historical thread comment" '.review_threads[] | select(.thread_id == "PRRT_1") | .comment_id == 111' "$json" @@ -516,9 +1152,10 @@ test_summary_mode_returns_compact_fixup_state() { assert_jq "summary keeps pr" '.pr.number == 123' "$json" assert_jq "summary keeps since" '.since.committed_at == "2026-06-01T12:00:00Z"' "$json" - assert_jq "summary failed check count" '.failed_checks | length == 2' "$json" + assert_jq "summary failed check count" '.failed_checks | length == 3' "$json" assert_jq "summary failed check" '.failed_checks[] | select(.name == "e2e") | .conclusion == "failure" and .run_id == "27340000002"' "$json" assert_jq "summary reports failed duplicate over skipped" '.failed_checks[] | select(.name == "opencode-review-fork") | .conclusion == "failure" and .run_id == "27340000007"' "$json" + assert_jq "summary retains failure before a duplicate cancellation" '.failed_checks[] | select(.name == "late-cancelled-failure") | .conclusion == "failure" and .run_id == "27340000008"' "$json" assert_jq "summary ignores cancelled duplicate check" '[.failed_checks[] | select(.name == "web lint")] | length == 0' "$json" assert_jq "summary dedupes skipped duplicate check" '[.failed_checks[], .pending_checks[] | select(.name == "opencode-review-same-repo")] | length == 0' "$json" assert_jq "summary pending check count" '.pending_checks | length == 2' "$json" @@ -626,6 +1263,28 @@ test_comment_mode_rejects_incompatible_flags() { } test_snapshot_happy_path +test_old_head_review_does_not_qualify +test_exact_head_selected_review_qualifies +test_clean_marker_requires_configured_dedicated_app_provenance +test_trusted_workflow_requires_latest_success_and_matching_marker +test_fork_and_unknown_provenance_are_untrusted +test_head_change_during_collection_invalidates_review_evidence +test_head_change_during_authenticated_workflow_collection_invalidates_trust +test_checks_head_mismatch_invalidates_combined_evidence +test_exact_head_dismissed_review_is_not_eligible +test_approved_review_with_blocker_is_not_eligible +test_commented_blocker_is_not_eligible +test_plain_blocker_labels_are_blocked_and_aggregated +test_colon_blocker_labels_are_line_anchored_and_scan_all_lines +test_formatted_colon_labels_handle_emphasis_after_colon +test_single_asterisk_emphasis_and_list_markers_are_safe +test_explicit_clean_commented_review_is_eligible +test_zero_blocker_summaries_remain_clean_with_explicit_marker +test_positive_blocker_evidence_overrides_clean_marker +test_exact_head_blockers_from_any_author_are_aggregated +test_fenced_blocker_examples_do_not_block_clean_review +test_latest_decisive_review_state_controls_active_change_requests +test_quoted_blocker_language_does_not_block_clean_review test_partial_failure_records_error_but_keeps_other_data test_pr_view_failure_with_non_numeric_ref_keeps_schema test_repo_failure_skips_review_threads