Repository navigation
Conversation
Reviewer's GuideImplements gate artifact extraction end-to-end: GitHub primitive ops now return structured artifacts (comment bodies and PR metadata), the gate evaluator and engine propagate and persist these artifacts on entities before state transitions, and the engineering flow’s spec-posted gate is wired to populate architectSpec from a matched spec comment; tests and docs are added to cover the new behavior and platform-core is bumped for auth cookie domain support. Sequence diagram for gate artifact extraction from spec-posted commentsequenceDiagram
actor SpecAgent
participant Engine
participant GateEvaluator
participant PrimitiveOpHandler
participant GitHub
participant EntityRepo
participant LlmCoder
SpecAgent->>Engine: signal spec_ready for entity
Engine->>GateEvaluator: evaluateGate spec_posted
GateEvaluator->>PrimitiveOpHandler: run primitiveOp issue_tracker.comment_exists
PrimitiveOpHandler->>GitHub: GET /issues/{issueNumber}/comments
GitHub-->>PrimitiveOpHandler: comments JSON
PrimitiveOpHandler->>PrimitiveOpHandler: find last body matching pattern
PrimitiveOpHandler-->>GateEvaluator: { outcome exists, artifacts { extractedBody } }
GateEvaluator-->>Engine: GateEvalResult { passed true, artifacts { extractedBody } }
Engine->>Engine: resolveGate returns { kind proceed, artifacts { extractedBody } }
Engine->>EntityRepo: updateArtifacts(entityId, { architectSpec })
Note over Engine,EntityRepo: artifactKey mapping replaces extractedBody with architectSpec
EntityRepo-->>Engine: updated entity
Engine-->>SpecAgent: gate passed, transition to next state
Engine->>LlmCoder: build prompt with entity.artifacts.architectSpec
LlmCoder-->>Engine: completion using extracted spec
Engine-->>SpecAgent: coding task results
Class diagram for gate evaluation and artifact propagationclassDiagram
class Engine {
+resolveGate(gateId, entity, flow, txRepos)
+_processSignalInner(signal, entityId, flow, txRepos)
}
class GateEvalResult {
+boolean passed
+boolean timedOut
+string output
+string outcome
+string message
+Record~string, unknown~ artifacts
}
class PrimitiveOpHandler {
<<interface>>
+handle(primitiveOp, primitiveParams, entity) Promise~PrimitiveOpResult~
}
class PrimitiveOpResult {
+string outcome
+string message
+Record~string, unknown~ artifacts
}
class GitHubPrimitiveOps {
+checkCiStatus(ctx, ref) Promise~PrimitiveOpResult~
+checkPrStatus(ctx, pullNumber) Promise~PrimitiveOpResult~
+checkCommentExists(ctx, issueNumber, pattern) Promise~PrimitiveOpResult~
}
class EntityRepo {
+updateArtifacts(entityId, artifacts)
+get(entityId) Entity
}
class Entity {
+string id
+Record~string, unknown~ artifacts
}
Engine --> GateEvalResult : uses
Engine --> PrimitiveOpHandler : calls
PrimitiveOpHandler --> PrimitiveOpResult : returns
Engine --> EntityRepo : updates artifacts
EntityRepo --> Entity : persists
GitHubPrimitiveOps ..|> PrimitiveOpHandler : primitive ops implementation
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoGate artifact extraction — Phase 1 of gate-driven signals
WalkthroughsDescription• Gates now extract and return artifacts alongside routing outcomes • checkCommentExists extracts matched comment body as extractedBody • checkPrStatus extracts PR metadata (url, number, headSha) as artifacts • Engine persists gate-extracted artifacts to entity before state transition • spec-posted gate maps extracted comment to architectSpec via artifactKey param • Added comprehensive tests for artifact extraction in gates and primitive ops Diagramflowchart LR
A["Gate Evaluation"] -->|"returns artifacts"| B["GateEvalResult"]
B -->|"propagates"| C["resolveGate"]
C -->|"persists"| D["Entity.artifacts"]
E["checkCommentExists"] -->|"extracts body"| F["extractedBody artifact"]
G["checkPrStatus"] -->|"extracts metadata"| H["prUrl, prNumber, headSha"]
F -->|"mapped via artifactKey"| I["architectSpec"]
File Changes1. src/engine/gate-evaluator.ts
|
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughAdds gate artifact extraction: gates and primitive ops can return optional Changes
Sequence DiagramsequenceDiagram
participant Engine as Engine
participant Evaluator as Gate Evaluator
participant Op as Primitive Op Handler
participant GitHub as GitHub API
participant Repo as Entity Repo
Engine->>Evaluator: evaluateGate(gate, context)
activate Evaluator
Evaluator->>Op: run primitive op (e.g., checkPrForBranch)
activate Op
Op->>GitHub: fetch PRs / comments / files
GitHub-->>Op: API response (JSON)
Op-->>Evaluator: GateOpResult { outcome, artifacts? }
deactivate Op
Evaluator-->>Engine: GateEvalResult { passed, outcome, artifacts? }
deactivate Evaluator
alt routing produces non-blocking proceed/redirect with artifacts
Engine->>Repo: updateArtifacts(entityId, routing.artifacts)
activate Repo
Repo-->>Engine: success
deactivate Repo
Engine->>Repo: fetch entity (refreshed for template rendering)
activate Repo
Repo-->>Engine: entity with persisted artifacts
deactivate Repo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
cookieDomaininitialization insrc/index.tsassumesproductDomainhas no leading dot; you may want to normalize (strip an existing dot) before prefixing to avoid generating invalid domains like..example.com. - In the
issue_tracker.comment_existshandler, you mutateresult.artifactsin place and assumeextractedBodyis a string; consider guarding with a type check and constructing a new artifacts object to avoid unexpected shapes leaking through from primitive ops. - The tests in
tests/github/primitive-ops.test.tsstub the globalfetchwithout restoring it; adding anafterAll/afterEachto restore the originalfetchwill reduce the risk of subtle cross-test interference as the suite grows.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `cookieDomain` initialization in `src/index.ts` assumes `productDomain` has no leading dot; you may want to normalize (strip an existing dot) before prefixing to avoid generating invalid domains like `..example.com`.
- In the `issue_tracker.comment_exists` handler, you mutate `result.artifacts` in place and assume `extractedBody` is a string; consider guarding with a type check and constructing a new artifacts object to avoid unexpected shapes leaking through from primitive ops.
- The tests in `tests/github/primitive-ops.test.ts` stub the global `fetch` without restoring it; adding an `afterAll`/`afterEach` to restore the original `fetch` will reduce the risk of subtle cross-test interference as the suite grows.
## Individual Comments
### Comment 1
<location path="src/index.ts" line_range="117-121" />
<code_context>
// ─── 4. BetterAuth init ───────────────────────────────────────────────
const { initBetterAuth, runAuthMigrations } = await import("@wopr-network/platform-core/auth/better-auth");
const { grantSignupCredits } = await import("@wopr-network/platform-core/credits");
+ const productDomain = container.productConfig.product?.domain;
initBetterAuth({
pool: container.pool,
db: platformDb,
+ cookieDomain: productDomain ? `.${productDomain}` : undefined,
onUserCreated: async (userId: string) => {
try {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Be defensive about cookie domain formatting, especially for localhost and already-dotted domains.
Using `.${productDomain}` assumes `productDomain` is never dotted and is always a proper DNS domain. If it’s `.example.com` you get `..example.com`, and for `localhost` you generally want no leading dot. Consider normalizing first: strip any existing leading dot, and skip adding a dot entirely for non-DNS-like values such as `localhost` before setting `cookieDomain`.
Suggested implementation:
```typescript
// ─── 4. BetterAuth init ───────────────────────────────────────────────
const { initBetterAuth, runAuthMigrations } = await import("@wopr-network/platform-core/auth/better-auth");
const { grantSignupCredits } = await import("@wopr-network/platform-core/credits");
const normalizeCookieDomain = (domain?: string | null): string | undefined => {
if (!domain) return undefined;
// Trim whitespace and leading dots
const trimmed = domain.trim().replace(/^\.+/, "");
if (!trimmed) return undefined;
// For localhost and other non-DNS-like hostnames (no dot), skip setting a cookie domain
if (!trimmed.includes(".")) return undefined;
// At this point we have something DNS-like, ensure a single leading dot
return `.${trimmed}`;
};
const productDomain = container.productConfig.product?.domain;
const cookieDomain = normalizeCookieDomain(productDomain);
initBetterAuth({
```
```typescript
pool: container.pool,
db: platformDb,
cookieDomain,
```
</issue_to_address>
### Comment 2
<location path="tests/github/primitive-ops.test.ts" line_range="68" />
<code_context>
+```typescript
+import { checkPrStatus } from "../../src/github/primitive-ops.js";
+
+describe("checkPrStatus", () => {
+ const ctx = { token: "test-token", owner: "wopr-network", repo: "test-repo" };
+
</code_context>
<issue_to_address>
**issue (testing):** Missing test for the `closed` (not merged) PR outcome in `checkPrStatus`
The current tests don’t hit the case where `pr.merged === false` and `pr.state === "closed"` (a PR closed without merging). Please add a test with `merged: false, state: "closed", mergeable_state: "unknown"` that asserts we return `outcome: "closed"` and still populate artifacts correctly, to guard against regressions for this path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const productDomain = container.productConfig.product?.domain; | ||
| initBetterAuth({ | ||
| pool: container.pool, | ||
| db: platformDb, | ||
| cookieDomain: productDomain ? `.${productDomain}` : undefined, |
There was a problem hiding this comment.
suggestion (bug_risk): Be defensive about cookie domain formatting, especially for localhost and already-dotted domains.
Using .${productDomain} assumes productDomain is never dotted and is always a proper DNS domain. If it’s .example.com you get ..example.com, and for localhost you generally want no leading dot. Consider normalizing first: strip any existing leading dot, and skip adding a dot entirely for non-DNS-like values such as localhost before setting cookieDomain.
Suggested implementation:
// ─── 4. BetterAuth init ───────────────────────────────────────────────
const { initBetterAuth, runAuthMigrations } = await import("@wopr-network/platform-core/auth/better-auth");
const { grantSignupCredits } = await import("@wopr-network/platform-core/credits");
const normalizeCookieDomain = (domain?: string | null): string | undefined => {
if (!domain) return undefined;
// Trim whitespace and leading dots
const trimmed = domain.trim().replace(/^\.+/, "");
if (!trimmed) return undefined;
// For localhost and other non-DNS-like hostnames (no dot), skip setting a cookie domain
if (!trimmed.includes(".")) return undefined;
// At this point we have something DNS-like, ensure a single leading dot
return `.${trimmed}`;
};
const productDomain = container.productConfig.product?.domain;
const cookieDomain = normalizeCookieDomain(productDomain);
initBetterAuth({ pool: container.pool,
db: platformDb,
cookieDomain,| }); | ||
| }); | ||
|
|
||
| describe("checkPrStatus", () => { |
There was a problem hiding this comment.
issue (testing): Missing test for the closed (not merged) PR outcome in checkPrStatus
The current tests don’t hit the case where pr.merged === false and pr.state === "closed" (a PR closed without merging). Please add a test with merged: false, state: "closed", mergeable_state: "unknown" that asserts we return outcome: "closed" and still populate artifacts correctly, to guard against regressions for this path.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| issueNumber: Number(params.issueNumber), | ||
| pattern: params.pattern as string, | ||
| }); | ||
| const artifactKey = params.artifactKey as string | undefined; |
There was a problem hiding this comment.
🟢 Low src/index.ts:345
When artifactKey is "extractedBody", the code assigns arts.extractedBody = arts.extractedBody (no-op), then immediately delete arts.extractedBody, dropping the artifact entirely. The check at line 347 doesn't prevent this because it only verifies that artifactKey and arts.extractedBody are truthy, not that they differ. Consider guarding the reassignment to skip the delete when the keys match.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/index.ts around line 345:
When `artifactKey` is `"extractedBody"`, the code assigns `arts.extractedBody = arts.extractedBody` (no-op), then immediately `delete arts.extractedBody`, dropping the artifact entirely. The check at line 347 doesn't prevent this because it only verifies that `artifactKey` and `arts.extractedBody` are truthy, not that they differ. Consider guarding the reassignment to skip the delete when the keys match.
Evidence trail:
src/index.ts lines 340-360 at REVIEWED_COMMIT - shows the artifact renaming logic without a guard for when artifactKey equals "extractedBody"
Greptile SummaryThis PR implements gate artifact extraction:
Confidence Score: 3/5Not safe to merge — two P1 defects on changed paths will silently drop a new gate and permanently misroute clean PRs to the fix state. Score of 3 reflects two current defects:
|
| Filename | Overview |
|---|---|
| src/engine/engine.ts | Artifact persistence added after gate resolution; null-guard for refreshed entity is asymmetric with pre-existing strict pattern (addressed in prior thread) |
| src/engine/gate-evaluator.ts | GateEvalResult and PrimitiveOpHandler extended with optional artifacts field; fully backward-compatible |
| src/flows/engineering.ts | Four new gates added; pr-exists shares {fromState, trigger} key with ci-green in GATE_WIRING and is silently never wired to any transition |
| src/github/primitive-ops.ts | Four new primitive ops added; bot-comment detection fires on any bot issue comment including benign summaries; no pagination past first 100 items |
| src/index.ts | New ops wired into primitiveOpHandler switch; issue_tracker.comment_exists artifact key remapping mutates returned object in-place |
| tests/github/primitive-ops.test.ts | Comprehensive unit tests for all new ops with mock fetch; covers happy path, not-found, and error cases |
| tests/engine/gate-artifact-extraction.test.ts | Contract tests for GateEvalResult artifacts shape; backward-compatibility verified |
| tests/flows/engineering-flow.test.ts | Gate count and spec-posted wiring tested; pr-exists dead-gate issue not caught by existing assertions |
Sequence Diagram
sequenceDiagram
participant Agent
participant Engine
participant GateEvaluator
participant PrimitiveOp
participant GitHub
participant EntityRepo
Agent->>Engine: processSignal(entityId, signal, artifacts?)
Engine->>EntityRepo: updateArtifacts(signal artifacts)
EntityRepo-->>Engine: refreshed entity
Engine->>GateEvaluator: evaluateGate(gate, entity)
GateEvaluator->>PrimitiveOp: handler(primitiveOp, params, entity)
PrimitiveOp->>GitHub: API call (check PR / comment)
GitHub-->>PrimitiveOp: response
PrimitiveOp-->>GateEvaluator: {outcome, artifacts}
GateEvaluator-->>Engine: GateEvalResult {passed, artifacts}
Engine->>EntityRepo: updateArtifacts(gate artifacts)
EntityRepo-->>Engine: refreshed entity
Engine->>Engine: transition to toState
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/flows/engineering.ts
Line: 388-395
Comment:
**`pr-exists` gate silently never wired to any transition**
`"ci-green"` and `"pr-exists"` both map to `{ fromState: "code", trigger: "pr_created" }` in `GATE_WIRING`. `provisionEngineeringFlow` resolves the gate for each transition with `Object.entries(GATE_WIRING).find(...)`, which returns the **first match only**. `"ci-green"` appears before `"pr-exists"` in the object literal, so it always wins — `"pr-exists"` is inserted into the DB but its `gateId` is never written onto any transition row. The gate is effectively dead.
Give `pr-exists` its own trigger so it gets a unique lookup:
```suggestion
export const GATE_WIRING: Record<string, { fromState: string; trigger: string }> = {
"spec-posted": { fromState: "spec", trigger: "spec_ready" },
"ci-green": { fromState: "code", trigger: "pr_created" },
"pr-exists": { fromState: "code", trigger: "pr_opened" },
"review-status": { fromState: "review", trigger: "clean" },
"pr-updated": { fromState: "fix", trigger: "fixes_pushed" },
"docs-committed": { fromState: "docs", trigger: "docs_ready" },
"pr-mergeable": { fromState: "merge", trigger: "merged" },
};
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/github/primitive-ops.ts
Line: 161-163
Comment:
**Any bot comment — including benign walkthrough summaries — permanently blocks the gate**
`botComments` collects every issue comment from a known bot login, with no regard for content or valence. CodeRabbit, Sourcery, and similar bots always post an initial PR walkthrough summary even when they find zero actionable issues. That single summary comment satisfies `botComments.length > 0` → `hasUnresolvedBotFindings = true` → `"has_issues"` outcome — routing clean PRs to the `fix` state indefinitely.
Consider filtering bot comments by body content (e.g. require presence of `"CHANGES_REQUESTED"` or `"action required"` keywords), or rely solely on the formal `CHANGES_REQUESTED` review state from the reviews endpoint rather than issue-level comments.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/github/primitive-ops.ts
Line: 93-100
Comment:
**Missing pagination — results capped at first 100 items**
`checkPrForBranch` fetches at most 100 open PRs and `checkFilesChangedSince` fetches at most 100 changed files; neither follows GitHub's `Link: <…>; rel="next"` header. In a busy repository (>100 open PRs) or a large PR (>100 changed files), the target item may be on page 2+ and the function silently returns `not_found` / `unchanged`.
How can I resolve this? If you propose a fix, please make it concise.Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.
Reviews (2): Last reviewed commit: "feat(engine): Phase 2 — 4 new primitive ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const productDomain = container.productConfig.product?.domain; | ||
| initBetterAuth({ | ||
| pool: container.pool, | ||
| db: platformDb, |
There was a problem hiding this comment.
The cookieDomain addition to initBetterAuth is unrelated to gate artifact extraction. It appears to be a separate auth/session fix. Bundling unrelated changes makes rollback harder and blurs the change's purpose. Ideally this should live in its own commit/PR, or at minimum be called out explicitly in the PR description.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 117-120
Comment:
**Unrelated change bundled into this PR**
The `cookieDomain` addition to `initBetterAuth` is unrelated to gate artifact extraction. It appears to be a separate auth/session fix. Bundling unrelated changes makes rollback harder and blurs the change's purpose. Ideally this should live in its own commit/PR, or at minimum be called out explicitly in the PR description.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
Adds gate-level artifact extraction and persistence so downstream states can reliably consume externally-derived artifacts (e.g., spec comment body, PR metadata) without depending on agent-emitted structured output.
Changes:
- Extend primitive gate ops (
checkCommentExists,checkPrStatus) to return extracted artifacts. - Propagate
artifactsthrough gate evaluation/routing and persist them onto the entity before state transition. - Update the engineering flow’s
spec-postedgate to map extracted comment body intoentity.artifacts.architectSpec, with new tests + docs.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/github/primitive-ops.test.ts | Adds unit tests for comment-body extraction and PR metadata extraction. |
| tests/flows/engineering-flow.test.ts | Asserts spec-posted gate has outcomes map and artifactKey. |
| tests/engine/gate-artifact-extraction.test.ts | Adds a contract test asserting GateEvalResult can carry artifacts. |
| src/index.ts | Remaps extracted comment body artifact into a configurable key (artifactKey). |
| src/github/primitive-ops.ts | Implements artifact extraction for issue comments and PR status. |
| src/flows/engineering.ts | Adds outcomes map and artifactKey for spec-posted gate. |
| src/engine/gate-evaluator.ts | Adds artifacts to GateEvalResult and primitive handler result propagation. |
| src/engine/engine.ts | Persists gate-extracted artifacts before state transition; returns artifacts in routing. |
| package.json | Bumps @wopr-network/platform-core to ^1.74.0. |
| pnpm-lock.yaml | Updates lockfile for the dependency bump (and transitive updates). |
| docs/specs/2026-04-01-gate-driven-signals-design.md | Documents the gate-driven signals architecture and artifact extraction contracts. |
| docs/plans/2026-04-01-gate-artifact-extraction.md | Implementation plan for Phase 1 artifact extraction/persistence. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/engine/gate-evaluator.ts:27
- The PrimitiveOpHandler docstring says it returns
{ outcome, message }, but the type now also allowsartifacts. Consider updating the comment to reflect the new contract so future primitive ops know artifacts can be returned/persisted.
/**
* Handler for primitive gate operations (e.g. vcs.ci_status, issue_tracker.comment_exists).
* Returns { outcome, message } matching the gate's outcomes map.
*/
export type PrimitiveOpHandler = (
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist gate-extracted artifacts (e.g., spec body, PR metadata) | ||
| if (routing.artifacts && Object.keys(routing.artifacts).length > 0) { | ||
| await entityRepo.updateArtifacts(entityId, routing.artifacts); | ||
| const refreshed = await entityRepo.get(entityId); | ||
| if (refreshed) entity = refreshed; |
There was a problem hiding this comment.
This method is transaction-aware (txRepos → local entityRepo), but the earlier “Persist signal artifacts BEFORE gate evaluation” block uses this.entityRepo/update+get, while this new gate-artifacts block uses the tx-scoped entityRepo. That can cause artifact writes/reads to escape the transaction and break atomicity. Use the local entityRepo consistently for both the pre-gate and post-gate artifact persistence/refresh.
| const artifactKey = params.artifactKey as string | undefined; | ||
| if (artifactKey && result.artifacts) { | ||
| const arts = result.artifacts as Record<string, unknown>; | ||
| if (arts.extractedBody) { |
There was a problem hiding this comment.
The remap uses a truthiness check (if (arts.extractedBody)) before copying/deleting. If the matched comment body is an empty string, the mapping won’t happen and the gate will persist extractedBody instead of the configured artifactKey. Prefer checking key presence ("extractedBody" in arts) or arts.extractedBody !== undefined (and optionally validating it’s a string) before remapping.
| if (arts.extractedBody) { | |
| if ("extractedBody" in arts) { |
| const res = await fetch( | ||
| `https://api.github.com/repos/${ctx.owner}/${ctx.repo}/issues/${params.issueNumber}/comments?per_page=100`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }, | ||
| ); | ||
| if (!res.ok) { | ||
| return { outcome: "error", message: `GitHub API error: ${res.status}` }; | ||
| } | ||
| const comments = (await res.json()) as Array<{ body: string }>; | ||
| const regex = new RegExp(params.pattern); | ||
| const found = comments.some((c) => regex.test(c.body)); | ||
| return found | ||
| ? { outcome: "exists", message: "Matching comment found" } | ||
| const match = comments.filter((c) => regex.test(c.body)).at(-1); | ||
| return match | ||
| ? { outcome: "exists", message: "Matching comment found", artifacts: { extractedBody: match.body } } | ||
| : { outcome: "not_found", message: "No matching comment" }; |
There was a problem hiding this comment.
checkCommentExists only fetches the first page of issue comments (per_page=100 with no pagination). For issues with >100 comments, the most recent matching spec comment may be on later pages, so “last match wins” can silently pick an outdated match or miss the match entirely. Consider paginating (follow Link headers) or otherwise ensuring you fetch the most recent comments before selecting the last match.
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { checkCommentExists, checkPrStatus } from "../../src/github/primitive-ops.js"; | ||
|
|
||
| const mockFetch = vi.fn(); | ||
| vi.stubGlobal("fetch", mockFetch); | ||
|
|
There was a problem hiding this comment.
This test file stubs globalThis.fetch once at module scope but never unstubs it. If Vitest runs files in the same environment (or isolation is disabled), the stub can leak into other test suites. Add an afterAll (or try/finally) that calls vi.unstubAllGlobals()/vi.unstubGlobal("fetch") to keep global state isolated.
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |
| import { checkCommentExists, checkPrStatus } from "../../src/github/primitive-ops.js"; | |
| const mockFetch = vi.fn(); | |
| vi.stubGlobal("fetch", mockFetch); | |
| import { describe, it, expect, vi, beforeEach, afterAll } from "vitest"; | |
| import { checkCommentExists, checkPrStatus } from "../../src/github/primitive-ops.js"; | |
| const mockFetch = vi.fn(); | |
| vi.stubGlobal("fetch", mockFetch); | |
| afterAll(() => { | |
| vi.unstubGlobal("fetch"); | |
| }); |
| // Gate passed or redirected — determine the actual destination. | ||
| let toState = routing.kind === "redirect" ? routing.toState : transition.toState; | ||
| const trigger = routing.kind === "redirect" ? routing.trigger : signal; |
There was a problem hiding this comment.
Gate artifacts are persisted here, but later the transition call still passes the original artifacts (signal/agent artifacts) into entityRepo.transition(...), which performs another shallow merge and can overwrite keys that the gate just extracted (if they overlap). Consider ensuring gate artifacts win (e.g., merge into the transition artifacts with gate values last) or avoid re-merging signal artifacts after gate persistence.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…sitions New GitHub primitive ops: - vcs.pr_for_branch — detect PR creation by branch pattern - vcs.pr_review_status — detect clean vs unresolved review comments/bot findings - vcs.pr_head_changed — detect fixes pushed (head SHA changed) - vcs.files_changed_since — detect docs committed (matching file patterns) All ops return extracted artifacts (PR metadata, review findings, changed files). 4 new gate definitions added to engineering flow with outcomes maps. Gate wiring updated for all new transitions. 17 new tests for primitive ops, 49 total tests passing. Phase 2 of #266 (gate-driven signals). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c6e5af0 to
4b6e69e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/github/primitive-ops.ts (1)
71-88:⚠️ Potential issue | 🟠 MajorImplement pagination to fetch all comment pages for accurate "last match" detection.
Line 71 fetches only the first page (
per_page=100) without pagination. On issues with >100 comments, this misses newer matching comments. GitHub REST API orders comments by ascending ID (oldest first), so the most recent matching comment may be on a later page. The current logic will persist stale artifacts instead of selecting the newest match.Suggested fix (paginate through all pages)
export async function checkCommentExists( ctx: GitHubGateContext, params: { issueNumber: number; pattern: string }, ): Promise<GateOpResult> { - const res = await fetch( - `https://api.github.com/repos/${ctx.owner}/${ctx.repo}/issues/${params.issueNumber}/comments?per_page=100`, - { - headers: { - Authorization: `Bearer ${ctx.token}`, - Accept: "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - }, - }, - ); - if (!res.ok) { - return { outcome: "error", message: `GitHub API error: ${res.status}` }; - } - const comments = (await res.json()) as Array<{ body: string }>; + const headers = { + Authorization: `Bearer ${ctx.token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; const regex = new RegExp(params.pattern); - const match = comments.filter((c) => regex.test(c.body)).at(-1); - return match + let page = 1; + let lastMatch: { body: string } | undefined; + + while (true) { + const res = await fetch( + `https://api.github.com/repos/${ctx.owner}/${ctx.repo}/issues/${params.issueNumber}/comments?per_page=100&page=${page}`, + { headers }, + ); + if (!res.ok) return { outcome: "error", message: `GitHub API error: ${res.status}` }; + + const comments = (await res.json()) as Array<{ body: string }>; + for (const c of comments) { + if (regex.test(c.body)) lastMatch = c; + } + if (comments.length < 100) break; + page++; + } + + return lastMatch - ? { outcome: "exists", message: "Matching comment found", artifacts: { extractedBody: match.body } } + ? { outcome: "exists", message: "Matching comment found", artifacts: { extractedBody: lastMatch.body } } : { outcome: "not_found", message: "No matching comment" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/github/primitive-ops.ts` around lines 71 - 88, The current fetch only gets a single page (per_page=100) so newer matching comments on later pages are missed; update the logic around the fetch call that uses ctx.owner, ctx.repo, params.issueNumber and ctx.token to paginate through all comment pages (e.g., loop fetching with page query param or follow the Link header) accumulating results into comments before creating regex = new RegExp(params.pattern) and finding the last match; stop when no more pages, preserve existing error handling (res.ok) and return the most-recent matching comment (the last matching element) as the artifact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/2026-04-01-gate-driven-signals-design.md`:
- Around line 74-77: The example uses comments.find(...) (first-match) but the
implemented contract is last-match-wins; update the snippet to select the last
comment whose body matches the regex (e.g., reverse iterate or use findLast) so
the returned artifacts.architectSpec comes from the final matching comment; keep
the same return shape ({ outcome: "exists", artifacts: { architectSpec:
match.body } } or { outcome: "not_found" }) and reference the same symbols
(comments, regex, match, architectSpec) so the doc matches runtime behavior.
In `@src/engine/gate-evaluator.ts`:
- Around line 19-20: The aggregation path in evaluateGateForAllRepos() rebuilds
gate results for PRs without propagating GateEvalResult.artifacts, so any
metadata extracted into entity.artifacts.prs gets dropped before
Engine.resolveGate() runs; update evaluateGateForAllRepos() to include artifacts
when constructing aggregated results—either accumulate artifacts per repo keyed
by PR and attach the merged artifact bag to each aggregated
GateEvalResult.artifacts, or explicitly forward the selected
entity.artifacts.prs into the new GateEvalResult you create (see
GateEvalResult.artifacts, evaluateGateForAllRepos(), Engine.resolveGate(), and
entity.artifacts.prs for where to apply the change).
In `@src/github/primitive-ops.ts`:
- Around line 61-62: The current branch that returns { outcome: "blocked", ... }
for any non-"clean" PR should be changed to treat transient GitHub
mergeable_state values as in-progress: if pr.mergeable_state is "unknown",
"unstable", or "behind" return { outcome: "pending", message: `PR state:
${pr.mergeable_state}`, artifacts: prArtifacts } (matching how checkCiStatus
uses "pending"); only return { outcome: "blocked", ... } for other non-clean
states. Update the logic around pr.mergeable_state (the lines that currently
check for "clean" and return blocked) to explicitly check these transient states
first and return pending.
In `@tests/github/primitive-ops.test.ts`:
- Around line 1-5: Add an afterAll cleanup that restores any global stubs so the
fetch stub does not leak between describe blocks; specifically, after calling
vi.stubGlobal("fetch", mockFetch) at module scope, add afterAll(() =>
vi.unstubAllGlobals()) to restore the original globalThis.fetch and avoid
cross-suite state leakage, referencing the mockFetch and vi.stubGlobal usage in
the test file.
---
Outside diff comments:
In `@src/github/primitive-ops.ts`:
- Around line 71-88: The current fetch only gets a single page (per_page=100) so
newer matching comments on later pages are missed; update the logic around the
fetch call that uses ctx.owner, ctx.repo, params.issueNumber and ctx.token to
paginate through all comment pages (e.g., loop fetching with page query param or
follow the Link header) accumulating results into comments before creating regex
= new RegExp(params.pattern) and finding the last match; stop when no more
pages, preserve existing error handling (res.ok) and return the most-recent
matching comment (the last matching element) as the artifact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5282e16a-1447-44fc-9a72-cc9eaf8e9c41
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
docs/plans/2026-04-01-gate-artifact-extraction.mddocs/specs/2026-04-01-gate-driven-signals-design.mdpackage.jsonsrc/engine/engine.tssrc/engine/gate-evaluator.tssrc/flows/engineering.tssrc/github/primitive-ops.tssrc/index.tstests/engine/gate-artifact-extraction.test.tstests/flows/engineering-flow.test.tstests/github/primitive-ops.test.ts
| const match = comments.find((c) => regex.test(c.body)); | ||
| return match | ||
| ? { outcome: "exists", artifacts: { architectSpec: match.body } } | ||
| : { outcome: "not_found" }; |
There was a problem hiding this comment.
Spec example is out of sync with the implemented matching strategy.
This snippet uses comments.find(...) (first match), but implementation now uses last-match-wins behavior. Please update this contract example to avoid guiding future changes in the wrong direction.
📝 Suggested doc correction
-const match = comments.find((c) => regex.test(c.body));
+const match = comments.filter((c) => regex.test(c.body)).at(-1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const match = comments.find((c) => regex.test(c.body)); | |
| return match | |
| ? { outcome: "exists", artifacts: { architectSpec: match.body } } | |
| : { outcome: "not_found" }; | |
| const match = comments.filter((c) => regex.test(c.body)).at(-1); | |
| return match | |
| ? { outcome: "exists", artifacts: { architectSpec: match.body } } | |
| : { outcome: "not_found" }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/2026-04-01-gate-driven-signals-design.md` around lines 74 - 77,
The example uses comments.find(...) (first-match) but the implemented contract
is last-match-wins; update the snippet to select the last comment whose body
matches the regex (e.g., reverse iterate or use findLast) so the returned
artifacts.architectSpec comes from the final matching comment; keep the same
return shape ({ outcome: "exists", artifacts: { architectSpec: match.body } } or
{ outcome: "not_found" }) and reference the same symbols (comments, regex,
match, architectSpec) so the doc matches runtime behavior.
| /** Artifacts extracted by the gate op (e.g., comment body, PR metadata). */ | ||
| artifacts?: Record<string, unknown>; |
There was a problem hiding this comment.
Propagate artifacts on the prs aggregation path.
This file now exposes GateEvalResult.artifacts, but evaluateGateForAllRepos() still reconstructs aggregate results at Lines 344-350 and 356-362 without copying them. Any gate evaluated against entity.artifacts.prs will therefore lose extracted metadata before Engine.resolveGate() can persist it. Please either aggregate artifacts per repo or explicitly forward the selected artifact bag instead of silently dropping it.
Also applies to: 31-31, 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engine/gate-evaluator.ts` around lines 19 - 20, The aggregation path in
evaluateGateForAllRepos() rebuilds gate results for PRs without propagating
GateEvalResult.artifacts, so any metadata extracted into entity.artifacts.prs
gets dropped before Engine.resolveGate() runs; update evaluateGateForAllRepos()
to include artifacts when constructing aggregated results—either accumulate
artifacts per repo keyed by PR and attach the merged artifact bag to each
aggregated GateEvalResult.artifacts, or explicitly forward the selected
entity.artifacts.prs into the new GateEvalResult you create (see
GateEvalResult.artifacts, evaluateGateForAllRepos(), Engine.resolveGate(), and
entity.artifacts.prs for where to apply the change).
| if (pr.mergeable_state === "clean") return { outcome: "mergeable", artifacts: prArtifacts }; | ||
| return { outcome: "blocked", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub pull request API: what are valid mergeable_statevalues, and which values are transient (e.g.,unknown) vs actual blocked/conflict states?
💡 Result:
The valid mergeable_state values for GitHub Pull Request REST API (GET /repos/{owner}/{repo}/pulls/{pull_number}) are: "unknown", "behind", "blocked", "clean", "has_lines", "unstable". Transient states (computation in progress or temporary): "unknown" (mergeability not yet computed; retry request after delay), "behind" (likely head branch behind base, transient until rebase), "unstable" (temporary instability, e.g. during checks). Actual blocked/conflict states: "blocked" (blocked by branch protection rules/status checks even if mergeable=true), "clean" (mergeable, no conflicts), "has_lines" (mergeable with conflicts that can be auto-resolved?). Note: Official docs only explicitly document mergeable (true/false/null) but not mergeable_state (undocumented). Values confirmed from real API responses in GitHub issues, Stack Overflow, and CLI source analysis. mergeable_state provides more granular info than mergeable: e.g. mergeable=true but state=blocked due to required reviews/statuses.
Citations:
- 1: Question - what mergeable_state: blocked means? pascalgn/automerge-action#122
- 2: https://stackoverflow.com/questions/30619549/why-does-github-api-return-an-unknown-mergeable-state-in-a-pull-request
- 3: https://github.apidog.io/api-3489530
- 4: https://2fd.github.io/graphdoc/github/pullrequeststate.doc.html
- 5: https://stackoverflow.com/questions/71272868/how-to-filter-in-graphql-github-api-by-mergeablestate
- 6: Output of
gh pr view PR --json mergeableincorrect cli/cli#9583
🏁 Script executed:
# Check if the coding guidelines mention anything about PR mergeability states
grep -r "mergeable_state\|mergeable\|transient" . --include="*.md" --include="*.ts" --include="*.js" 2>/dev/null | head -20Repository: wopr-network/holyship
Length of output: 2145
🏁 Script executed:
# Check the actual implementation to understand what other states might be relevant
cd src/github && find . -name "*.ts" -type f | head -10Repository: wopr-network/holyship
Length of output: 140
🏁 Script executed:
# Examine the primitive-ops.ts file around the lines in question
cat -n src/github/primitive-ops.ts | head -100Repository: wopr-network/holyship
Length of output: 4371
🏁 Script executed:
# Check the test/spec file to see what outcomes are expected
grep -n "mergeable_state\|outcome" docs/plans/2026-04-01-gate-artifact-extraction.md | head -30Repository: wopr-network/holyship
Length of output: 2180
🏁 Script executed:
# Check the specs document to understand the full design
cat docs/specs/2026-04-01-gate-driven-signals-design.md | grep -A 10 -B 10 "mergeable\|transient\|pending"Repository: wopr-network/holyship
Length of output: 2684
🏁 Script executed:
# Check if there are any tests that show expected behavior for different mergeable_state values
grep -A 20 "mergeable_state.*unknown\|mergeable_state.*unstable\|mergeable_state.*behind" docs/plans/2026-04-01-gate-artifact-extraction.mdRepository: wopr-network/holyship
Length of output: 750
🏁 Script executed:
# Check the full test file to understand all test cases for checkPrStatus
grep -B 5 -A 15 "checkPrStatus\|pr-mergeable" docs/plans/2026-04-01-gate-artifact-extraction.md | head -80Repository: wopr-network/holyship
Length of output: 2367
🏁 Script executed:
# Let me check what test cases exist for different mergeable_state values
grep -n "mergeable_state" docs/plans/2026-04-01-gate-artifact-extraction.mdRepository: wopr-network/holyship
Length of output: 419
🏁 Script executed:
# Check if there are any other references to these transient states or if there's any discussion about handling them
grep -r "unknown\|unstable\|behind" src/flows --include="*.ts" -B 2 -A 2Repository: wopr-network/holyship
Length of output: 5470
🏁 Script executed:
# Check if the engineering flow or other docs mention handling of transient PR states
grep -r "transient\|pending.*merge\|still computing" docs --include="*.md" -B 2 -A 2Repository: wopr-network/holyship
Length of output: 47
🏁 Script executed:
# Check what outcomes are defined or expected by the system
grep -r "outcome.*pending\|pending.*outcome" src/ --include="*.ts" -B 2 -A 2Repository: wopr-network/holyship
Length of output: 1292
🏁 Script executed:
# Look at the GateOpResult type definition and check what outcomes are validated
grep -A 5 "type GateOpResult\|export.*outcome" src/github/primitive-ops.tsRepository: wopr-network/holyship
Length of output: 447
🏁 Script executed:
# Check what the test file expects for the "unknown" case
cat -n docs/plans/2026-04-01-gate-artifact-extraction.md | sed -n '298,310p'Repository: wopr-network/holyship
Length of output: 541
🏁 Script executed:
# Check if there are any other references to outcomes or states we should understand
grep -n "outcome.*blocked\|blocked.*outcome" src/github/primitive-ops.ts -B 2 -A 2Repository: wopr-network/holyship
Length of output: 361
🏁 Script executed:
# Verify the exact line numbers and context around the code under review
cat -n src/github/primitive-ops.ts | sed -n '58,65p'Repository: wopr-network/holyship
Length of output: 668
Handle transient PR mergeability states (unknown, unstable) as pending, not blocked.
Line 62 currently classifies every non-clean open PR as blocked. GitHub can return transient mergeability states—specifically unknown (computation in progress), unstable (temporary instability), and behind (during rebase)—which should not immediately trigger blocked routing. The system already uses the pending outcome for in-progress CI checks (see checkCiStatus); PR mergeability should follow the same pattern.
💡 Suggested fix (explicit pending branch)
const prArtifacts = { prUrl: pr.html_url, prNumber: pr.number, headSha: pr.head.sha };
if (pr.merged) return { outcome: "merged", artifacts: prArtifacts };
if (pr.state === "closed") return { outcome: "closed", artifacts: prArtifacts };
+ if (pr.mergeable_state === "unknown" || pr.mergeable_state === "unstable") {
+ return { outcome: "pending", message: "PR mergeability still computing", artifacts: prArtifacts };
+ }
if (pr.mergeable_state === "clean") return { outcome: "mergeable", artifacts: prArtifacts };
return { outcome: "blocked", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pr.mergeable_state === "clean") return { outcome: "mergeable", artifacts: prArtifacts }; | |
| return { outcome: "blocked", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts }; | |
| if (pr.mergeable_state === "unknown" || pr.mergeable_state === "unstable") { | |
| return { outcome: "pending", message: "PR mergeability still computing", artifacts: prArtifacts }; | |
| } | |
| if (pr.mergeable_state === "clean") return { outcome: "mergeable", artifacts: prArtifacts }; | |
| return { outcome: "blocked", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/github/primitive-ops.ts` around lines 61 - 62, The current branch that
returns { outcome: "blocked", ... } for any non-"clean" PR should be changed to
treat transient GitHub mergeable_state values as in-progress: if
pr.mergeable_state is "unknown", "unstable", or "behind" return { outcome:
"pending", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts }
(matching how checkCiStatus uses "pending"); only return { outcome: "blocked",
... } for other non-clean states. Update the logic around pr.mergeable_state
(the lines that currently check for "clean" and return blocked) to explicitly
check these transient states first and return pending.
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { checkCommentExists, checkPrStatus } from "../../src/github/primitive-ops.js"; | ||
|
|
||
| const mockFetch = vi.fn(); | ||
| vi.stubGlobal("fetch", mockFetch); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the full test file to see the complete context
cat -n tests/github/primitive-ops.test.tsRepository: wopr-network/holyship
Length of output: 5547
🏁 Script executed:
# Search for other test files that might be affected by this global stub
find tests -name "*.test.ts" -o -name "*.test.js" | head -20Repository: wopr-network/holyship
Length of output: 831
🌐 Web query:
Vitest vi.stubGlobal cleanup afterAll vi.unstubAllGlobals test isolation
💡 Result:
To properly clean up after using vi.stubGlobal in Vitest and ensure test isolation when mocking globals, use vi.unstubAllGlobals in teardown hooks. By default, Vitest does not automatically reset globals stubbed with vi.stubGlobal, so manual cleanup is required to prevent state leakage between tests. ## Recommended Approaches 1. Manual Cleanup with Hooks (Per-test or Per-suite): - Use afterEach for cleanup after every test (best for isolation). - Use afterAll for cleanup after all tests in a suite (lighter but riskier if tests run concurrently). Example with afterEach: import { test, expect, afterEach, vi } from 'vitest' afterEach( => { vi.unstubAllGlobals }) test('uses stubbed global', => { vi.stubGlobal('fetch', vi.fn) // Test code... }) Example with afterAll: import { test, expect, afterAll, vi } from 'vitest' test('uses stubbed global', => { vi.stubGlobal('fetch', vi.fn) // Test code... }) afterAll( => { vi.unstubAllGlobals }) 2. Automatic Cleanup via Config: - Enable unstubGlobals: true in vitest.config.js. Vitest will call vi.unstubAllGlobals before each test automatically. import { defineConfig } from 'vitest/config' export default defineConfig({ test: { unstubGlobals: true } }) Warning: May cause issues with async concurrent tests, as one test's completion restores globals used by others. ## Test Isolation Context - Vitest isolates test files by default (each runs in separate worker/process), preventing cross-file global leakage. - Within a single file, without cleanup, stubs persist across tests, breaking isolation. - For full isolation without manual cleanup, rely on Vitest's default isolate: true and use per-test hooks. - vi.unstubAllGlobals restores originals on globalThis, window (jsdom), and handles fake timers (v0.26.0+). ## Best Practices - Prefer afterEach over afterAll for strongest isolation. - Combine with vi.restoreAllMocks if using vi.spyOn alongside stubs. - Avoid isolate: false unless performance-critical, as it shares context across files. - For fake timers: vi.unstubAllGlobals resets them (post-v0.26.0); older versions need vi.useRealTimers. This ensures clean state, preventing flaky tests from global pollution.
Citations:
- 1: https://vitest.dev/guide/mocking/globals
- 2: https://main.vitest.dev/guide/mocking/globals
- 3: https://main.vitest.dev/config/unstubglobals
- 4: https://runebook.dev/en/articles/vitest/api/vi/vi-unstuballglobals-0-26-0
- 5: https://runebook.dev/en/articles/vitest/api/vi/vi-unstuballglobals
- 6: https://v3.vitest.dev/api/vi
- 7: How to reset globals after using stubGlobal? vitest-dev/vitest#1741
- 8: https://vitest.dev/guide/improving-performance
- 9: https://vitest.dev/guide/lifecycle
Add an afterAll cleanup hook to restore the fetch stub.
vi.stubGlobal("fetch", mockFetch) at module scope persists across both test suites in this file. While vi.clearAllMocks() resets call history before each test, it does not restore the original globalThis.fetch. This can cause the second describe block to inherit state from the first. Add afterAll(() => vi.unstubAllGlobals()) to clean up after all tests complete.
♻️ Proposed fix
-import { describe, it, expect, vi, beforeEach } from "vitest";
+import { describe, it, expect, vi, beforeEach, afterAll } from "vitest";
import { checkCommentExists, checkPrStatus } from "../../src/github/primitive-ops.js";
const mockFetch = vi.fn();
vi.stubGlobal("fetch", mockFetch);
+
+afterAll(() => {
+ vi.unstubAllGlobals();
+});Also applies to: 10-12
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/github/primitive-ops.test.ts` around lines 1 - 5, Add an afterAll
cleanup that restores any global stubs so the fetch stub does not leak between
describe blocks; specifically, after calling vi.stubGlobal("fetch", mockFetch)
at module scope, add afterAll(() => vi.unstubAllGlobals()) to restore the
original globalThis.fetch and avoid cross-suite state leakage, referencing the
mockFetch and vi.stubGlobal usage in the test file.
| @@ -324,5 +387,9 @@ export const TRANSITIONS: CreateTransitionInput[] = [ | |||
| export const GATE_WIRING: Record<string, { fromState: string; trigger: string }> = { | |||
There was a problem hiding this comment.
🟡 Medium flows/engineering.ts:387
Both ci-green and pr-exists in GATE_WIRING map to the same transition (fromState: "code", trigger: "pr_created"). The provisioning code uses Object.entries(GATE_WIRING).find(...) which returns only the first match, so pr-exists is created but never wired to any transition — the PR existence check is silently unenforced. Consider using a data structure that supports multiple gates per transition, or add a second transition entry if the system supports one-to-many gate relationships.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/flows/engineering.ts around line 387:
Both `ci-green` and `pr-exists` in `GATE_WIRING` map to the same transition (`fromState: "code"`, `trigger: "pr_created"`). The provisioning code uses `Object.entries(GATE_WIRING).find(...)` which returns only the first match, so `pr-exists` is created but never wired to any transition — the PR existence check is silently unenforced. Consider using a data structure that supports multiple gates per transition, or add a second transition entry if the system supports one-to-many gate relationships.
Evidence trail:
src/flows/engineering.ts:389-390 (GATE_WIRING showing both ci-green and pr-exists mapping to same transition), src/flows/engineering.ts:360 (single transition with fromState: 'code', trigger: 'pr_created'), src/flows/engineering.ts:260,294 (both gates defined in GATES array), src/flows/provision.ts:48-55 (provisioning loop using Object.entries(GATE_WIRING).find() which returns only first match)
| const botPatterns = /qodo-code-review|coderabbitai|sourcery-ai|devin-ai/i; | ||
| const botComments = comments.filter((c) => botPatterns.test(c.user.login)); | ||
| const hasUnresolvedBotFindings = botComments.length > 0; | ||
|
|
||
| const findings = [ | ||
| ...reviews.filter((r) => r.state === "CHANGES_REQUESTED").map((r) => `[${r.user.login}] ${r.body}`), | ||
| ...botComments.map((c) => `[${c.user.login}] ${c.body}`), | ||
| ].join("\n\n---\n\n"); |
There was a problem hiding this comment.
🟡 Medium github/primitive-ops.ts:161
Accessing c.user.login and r.user.login crashes when user is null. GitHub returns user: null for comments/reviews from deleted ("ghost") accounts, so checkPrReviewStatus throws "Cannot read property 'login' of null". Consider using optional chaining or filtering out null users.
- const botPatterns = /qodo-code-review|coderabbitai|sourcery-ai|devin-ai/i;
- const botComments = comments.filter((c) => botPatterns.test(c.user.login));
- const hasUnresolvedBotFindings = botComments.length > 0;
-
- const findings = [
- ...reviews.filter((r) => r.state === "CHANGES_REQUESTED").map((r) => `[${r.user.login}] ${r.body}`),
- ...botComments.map((c) => `[${c.user.login}] ${c.body}`),
+ const botPatterns = /qodo-code-review|coderabbitai|sourcery-ai|devin-ai/i;
+ const botComments = comments.filter((c) => botPatterns.test(c.user?.login ?? ""));
+ const hasUnresolvedBotFindings = botComments.length > 0;
+
+ const findings = [
+ ...reviews.filter((r) => r.state === "CHANGES_REQUESTED").map((r) => `[${r.user?.login ?? "ghost"}] ${r.body}`),
+ ...botComments.map((c) => `[${c.user?.login ?? "ghost"}] ${c.body}`),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/github/primitive-ops.ts around lines 161-168:
Accessing `c.user.login` and `r.user.login` crashes when `user` is `null`. GitHub returns `user: null` for comments/reviews from deleted ("ghost") accounts, so `checkPrReviewStatus` throws "Cannot read property 'login' of null". Consider using optional chaining or filtering out null users.
Evidence trail:
src/github/primitive-ops.ts lines 155-166 (REVIEWED_COMMIT) - shows type assertions without null handling and direct property access on `c.user.login` and `r.user.login`. GitHub REST API schema issue: https://github.com/github/rest-api-description/issues/916 confirms `user` can be null for deleted accounts. GitHub community discussion: https://github.com/orgs/community/discussions/24888 confirms GraphQL returns null for deleted user authors.
| if (pattern.includes("*")) { | ||
| const regex = new RegExp(`^${pattern.replace(/\*/g, ".*")}$`); | ||
| return regex.test(f.filename); |
There was a problem hiding this comment.
🟢 Low github/primitive-ops.ts:230
The glob-to-regex conversion on line 231 only escapes *, so special regex characters in the pattern are interpreted literally. A pattern like src/file.ts matches src/fileXts because the unescaped . is treated as "any character". Consider escaping all regex metacharacters before converting * to .*.
if (pattern.includes("*")) {
- const regex = new RegExp(`^${pattern.replace(/\*/g, ".*")}$`);
+ const regex = new RegExp(`^${pattern.replace(/[.+^${}()|[\]\\]/g, "\\$&").replace(/\*/g, ".*")}$`);
return regex.test(f.filename);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file src/github/primitive-ops.ts around lines 230-232:
The glob-to-regex conversion on line 231 only escapes `*`, so special regex characters in the pattern are interpreted literally. A pattern like `src/file.ts` matches `src/fileXts` because the unescaped `.` is treated as "any character". Consider escaping all regex metacharacters before converting `*` to `.*`.
Evidence trail:
src/github/primitive-ops.ts lines 230-232 at REVIEWED_COMMIT: `const regex = new RegExp(\`^${pattern.replace(/\*/g, ".*")}$\`);` shows the glob-to-regex conversion only replaces `*` with `.*` without escaping other regex metacharacters.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/github/primitive-ops.ts (1)
70-88:⚠️ Potential issue | 🟠 Major
last match winscurrently means "last match on page 1."The issue-comments endpoint returns comments in ascending ID order and paginates results with
per_page/page(max 100 per page). Because this code hard-codesper_page=100and never follows pagination,filter(...).at(-1)only finds the newest match inside the first page. On busy issues, a reposted spec after comment 100 will be missed or an older body will be extracted.Similar pagination gaps exist in
checkPrForBranch()(line 96) andcheckFilesChangedSince()(line 213).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/github/primitive-ops.ts` around lines 70 - 88, The current comment-search logic in the comments fetch block only requests one page (per_page=100) and thus `filter(...).at(-1)` returns the last match on the first page only; fix this by implementing pagination for the GitHub comments endpoint inside the function that contains this block (look for the fetch call and variables ctx, params, and the use of regex and match), iterating pages (page=1..N) until an empty page or less-than-per-page result is returned, accumulate matching comments across all pages and then pick the final (most recent) match across the full set; apply the same pagination fix pattern to the other functions named checkPrForBranch and checkFilesChangedSince which currently request single pages and may miss results beyond the first page.
♻️ Duplicate comments (2)
docs/specs/2026-04-01-gate-driven-signals-design.md (1)
71-77:⚠️ Potential issue | 🟡 MinorSync this example with the current primitive-op contract.
The runtime now selects the last matching comment and returns it as
artifacts.extractedBody; the remap toarchitectSpechappens later insrc/index.ts. This snippet still documents first-match-wins and the wrong artifact shape, which will mislead future edits.📝 Suggested doc fix
-const match = comments.find((c) => regex.test(c.body)); +const match = comments.filter((c) => regex.test(c.body)).at(-1); return match - ? { outcome: "exists", artifacts: { architectSpec: match.body } } + ? { outcome: "exists", artifacts: { extractedBody: match.body } } : { outcome: "not_found" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/2026-04-01-gate-driven-signals-design.md` around lines 71 - 77, The example in the checkCommentExists snippet documents first-match semantics and returns artifacts.architectSpec, which is outdated; update the example to show finding the last matching comment (not using comments.find/first-match) and returning the current artifact shape artifacts.extractedBody (the remap to architectSpec happens later in src/index.ts). Specifically, replace the comments.find/first-match description with logic that selects the last matching comment and show the returned object using outcome: "exists" and artifacts: { extractedBody: <commentBody> } so the doc matches the runtime primitive-op contract.src/github/primitive-ops.ts (1)
50-62:⚠️ Potential issue | 🟠 MajorDon't collapse async mergeability checks into
blocked.GitHub documents PR mergeability as
true | false | null, withnullmeaning a background job is still computing the result. This branch ignoresmergeableentirely and turns every non-cleanopen PR intoblocked, so transient PRs can be bounced intofixeven though nothing is wrong yet. Add an explicit pending/retry outcome before the final fallback, and wire that outcome inpr-mergeable. (docs.github.com)💡 Minimal fix
const pr = (await res.json()) as { + mergeable: boolean | null; merged: boolean; state: string; mergeable_state: string; html_url: string; number: number; head: { sha: string }; }; const prArtifacts = { prUrl: pr.html_url, prNumber: pr.number, headSha: pr.head.sha }; if (pr.merged) return { outcome: "merged", artifacts: prArtifacts }; if (pr.state === "closed") return { outcome: "closed", artifacts: prArtifacts }; + if (pr.mergeable === null) { + return { outcome: "pending", message: "PR mergeability still computing", artifacts: prArtifacts }; + } if (pr.mergeable_state === "clean") return { outcome: "mergeable", artifacts: prArtifacts }; return { outcome: "blocked", message: `PR state: ${pr.mergeable_state}`, artifacts: prArtifacts };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/github/primitive-ops.ts` around lines 50 - 62, The code collapses all non-"clean" open PRs into "blocked" without checking GitHub's pr.mergeable tri-state; modify the branch that inspects the fetched PR (the pr object) to explicitly handle pr.mergeable === null by returning a pending/retry outcome (e.g., { outcome: "pending" } or { outcome: "retry" }) before falling back to "blocked", keep the existing checks for pr.merged, pr.state === "closed", and pr.mergeable_state === "clean", and ensure whatever outcome string you choose is also wired into the pr-mergeable handling code so the rest of the pipeline retries when mergeability is still being computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/2026-04-01-phase2-new-primitive-ops.md`:
- Around line 13-23: Update the Markdown task headings from level 3 to level 2
to fix the MD001 lint warning: change each "### Task X: `vcs.<op>` — ..." to "##
Task X: `vcs.<op>` — ..." for all Task 1..6 entries (references: Task 1
`vcs.pr_for_branch`, Task 2 `vcs.pr_review_status`, Task 3
`vcs.pr_head_changed`, Task 4 `vcs.files_changed_since`, Task 5
primitiveOpHandler/gates, Task 6 Tests + check + push) so the document goes from
the top-level `#` heading to `##` subsections and becomes lint-clean.
In `@src/flows/engineering.ts`:
- Around line 389-393: GATE_WIRING currently maps both gate IDs "ci-green" and
"pr-exists" to the same transition key (fromState: "code", trigger:
"pr_created") which overwrites one gate because the transition model stores a
single gateId; update the wiring so each gate has a unique transition entry —
either create a new intermediate state/transition (e.g., introduce "pr-extract"
or "ci-verify" states) so "ci-green" and "pr-exists" are on distinct
transitions, or implement an explicit composed gate entry (e.g., a new gateId
like "pr-ci-composite") and map that single composite gate to the (fromState:
"code", trigger: "pr_created") transition; make the change in the GATE_WIRING
definition and any provisioning logic that reads gateId so the engine no longer
loses one of the gates.
In `@src/github/primitive-ops.ts`:
- Around line 208-244: The current checkFilesChangedSince implementation only
inspects the PR's current file list and does not compare against a baseline SHA;
update checkFilesChangedSince to accept a baseline (e.g., add params.sinceSha or
params.baseSha) and use the GitHub compare API (or commit diff between base and
head) to fetch files changed between that baseline and the PR head instead of
using the pulls/files list, then apply the existing params.pathPatterns matching
logic to the compare result; alternatively, if you prefer not to make it
incremental, rename the function and relevant exports to reflect it only checks
current PR files (e.g., checkFilesInPR) and update callers accordingly.
- Around line 132-178: The code incorrectly treats any historical
CHANGES_REQUESTED as still blocking and treats all bot comments as unresolved;
fix by deduplicating reviews to only the latest review per user (use the reviews
array and group by r.user.login, keeping the item with the newest submitted_at)
and compute changesRequested from that deduped set (replace reviews.some(...)
and the CHANGES_REQUESTED filter in findings with the deduped list), and make
bot comments considered unresolved only if they are newer than the PR’s latest
update (fetch the pull via GET /pulls/{pull_number} to get pull.updated_at and
filter comments by comment.created_at >= pull.updated_at before computing
botComments and hasUnresolvedBotFindings); update findings construction to use
the deduped reviews and the filtered botComments (references: reviewsRes,
commentsRes, reviews, comments, changesRequested, botComments,
hasUnresolvedBotFindings, findings).
---
Outside diff comments:
In `@src/github/primitive-ops.ts`:
- Around line 70-88: The current comment-search logic in the comments fetch
block only requests one page (per_page=100) and thus `filter(...).at(-1)`
returns the last match on the first page only; fix this by implementing
pagination for the GitHub comments endpoint inside the function that contains
this block (look for the fetch call and variables ctx, params, and the use of
regex and match), iterating pages (page=1..N) until an empty page or
less-than-per-page result is returned, accumulate matching comments across all
pages and then pick the final (most recent) match across the full set; apply the
same pagination fix pattern to the other functions named checkPrForBranch and
checkFilesChangedSince which currently request single pages and may miss results
beyond the first page.
---
Duplicate comments:
In `@docs/specs/2026-04-01-gate-driven-signals-design.md`:
- Around line 71-77: The example in the checkCommentExists snippet documents
first-match semantics and returns artifacts.architectSpec, which is outdated;
update the example to show finding the last matching comment (not using
comments.find/first-match) and returning the current artifact shape
artifacts.extractedBody (the remap to architectSpec happens later in
src/index.ts). Specifically, replace the comments.find/first-match description
with logic that selects the last matching comment and show the returned object
using outcome: "exists" and artifacts: { extractedBody: <commentBody> } so the
doc matches the runtime primitive-op contract.
In `@src/github/primitive-ops.ts`:
- Around line 50-62: The code collapses all non-"clean" open PRs into "blocked"
without checking GitHub's pr.mergeable tri-state; modify the branch that
inspects the fetched PR (the pr object) to explicitly handle pr.mergeable ===
null by returning a pending/retry outcome (e.g., { outcome: "pending" } or {
outcome: "retry" }) before falling back to "blocked", keep the existing checks
for pr.merged, pr.state === "closed", and pr.mergeable_state === "clean", and
ensure whatever outcome string you choose is also wired into the pr-mergeable
handling code so the rest of the pipeline retries when mergeability is still
being computed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0a7157c-25bd-4c6b-b21d-4a036e58dd7e
📒 Files selected for processing (11)
docs/plans/2026-04-01-gate-artifact-extraction.mddocs/plans/2026-04-01-phase2-new-primitive-ops.mddocs/specs/2026-04-01-gate-driven-signals-design.mdsrc/engine/engine.tssrc/engine/gate-evaluator.tssrc/flows/engineering.tssrc/github/primitive-ops.tssrc/index.tstests/engine/gate-artifact-extraction.test.tstests/flows/engineering-flow.test.tstests/github/primitive-ops.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/engine/gate-artifact-extraction.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/flows/engineering-flow.test.ts
- src/engine/gate-evaluator.ts
- src/engine/engine.ts
| ### Task 1: `vcs.pr_for_branch` — detect PR creation | ||
|
|
||
| ### Task 2: `vcs.pr_review_status` — detect clean vs issues | ||
|
|
||
| ### Task 3: `vcs.pr_head_changed` — detect fixes pushed | ||
|
|
||
| ### Task 4: `vcs.files_changed_since` — detect docs committed | ||
|
|
||
| ### Task 5: Wire all 4 ops into primitiveOpHandler + add gate definitions | ||
|
|
||
| ### Task 6: Tests + check + push |
There was a problem hiding this comment.
Promote the task headings to ##.
The document jumps from the top-level # heading straight to ### Task ..., which is the MD001 heading-increment warning static analysis reported. Bumping these task sections to ## keeps the doc lint-clean.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-04-01-phase2-new-primitive-ops.md` around lines 13 - 23,
Update the Markdown task headings from level 3 to level 2 to fix the MD001 lint
warning: change each "### Task X: `vcs.<op>` — ..." to "## Task X: `vcs.<op>` —
..." for all Task 1..6 entries (references: Task 1 `vcs.pr_for_branch`, Task 2
`vcs.pr_review_status`, Task 3 `vcs.pr_head_changed`, Task 4
`vcs.files_changed_since`, Task 5 primitiveOpHandler/gates, Task 6 Tests + check
+ push) so the document goes from the top-level `#` heading to `##` subsections
and becomes lint-clean.
| "ci-green": { fromState: "code", trigger: "pr_created" }, | ||
| "pr-exists": { fromState: "code", trigger: "pr_created" }, | ||
| "review-status": { fromState: "review", trigger: "clean" }, | ||
| "pr-updated": { fromState: "fix", trigger: "fixes_pushed" }, | ||
| "docs-committed": { fromState: "docs", trigger: "docs_ready" }, |
There was a problem hiding this comment.
Only one gate can actually guard code -> pr_created.
GATE_WIRING now maps both ci-green and pr-exists to { fromState: "code", trigger: "pr_created" }, but the engine transition model only stores a single gateId per transition. One of these gates will overwrite the other during provisioning, so this hop will either lose PR extraction or lose CI validation. Split this into separate transitions/states, or add explicit multi-gate composition before wiring both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flows/engineering.ts` around lines 389 - 393, GATE_WIRING currently maps
both gate IDs "ci-green" and "pr-exists" to the same transition key (fromState:
"code", trigger: "pr_created") which overwrites one gate because the transition
model stores a single gateId; update the wiring so each gate has a unique
transition entry — either create a new intermediate state/transition (e.g.,
introduce "pr-extract" or "ci-verify" states) so "ci-green" and "pr-exists" are
on distinct transitions, or implement an explicit composed gate entry (e.g., a
new gateId like "pr-ci-composite") and map that single composite gate to the
(fromState: "code", trigger: "pr_created") transition; make the change in the
GATE_WIRING definition and any provisioning logic that reads gateId so the
engine no longer loses one of the gates.
| const [reviewsRes, commentsRes] = await Promise.all([ | ||
| fetch(`https://api.github.com/repos/${ctx.owner}/${ctx.repo}/pulls/${params.pullNumber}/reviews`, { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }), | ||
| fetch(`https://api.github.com/repos/${ctx.owner}/${ctx.repo}/issues/${params.pullNumber}/comments?per_page=100`, { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| if (!reviewsRes.ok || !commentsRes.ok) { | ||
| return { | ||
| outcome: "error", | ||
| message: `GitHub API error: reviews=${reviewsRes.status} comments=${commentsRes.status}`, | ||
| }; | ||
| } | ||
|
|
||
| const reviews = (await reviewsRes.json()) as Array<{ state: string; user: { login: string }; body: string }>; | ||
| const comments = (await commentsRes.json()) as Array<{ user: { login: string }; body: string }>; | ||
|
|
||
| const changesRequested = reviews.some((r) => r.state === "CHANGES_REQUESTED"); | ||
|
|
||
| const botPatterns = /qodo-code-review|coderabbitai|sourcery-ai|devin-ai/i; | ||
| const botComments = comments.filter((c) => botPatterns.test(c.user.login)); | ||
| const hasUnresolvedBotFindings = botComments.length > 0; | ||
|
|
||
| const findings = [ | ||
| ...reviews.filter((r) => r.state === "CHANGES_REQUESTED").map((r) => `[${r.user.login}] ${r.body}`), | ||
| ...botComments.map((c) => `[${c.user.login}] ${c.body}`), | ||
| ].join("\n\n---\n\n"); | ||
|
|
||
| if (changesRequested || hasUnresolvedBotFindings) { | ||
| return { | ||
| outcome: "has_issues", | ||
| message: `PR has ${changesRequested ? "changes requested" : ""}${changesRequested && hasUnresolvedBotFindings ? " and " : ""}${hasUnresolvedBotFindings ? `${botComments.length} bot findings` : ""}`, | ||
| artifacts: { reviewFindings: findings }, | ||
| }; | ||
| } | ||
|
|
||
| return { outcome: "clean", message: "No unresolved review comments" }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub REST API pull request reviews state CHANGES_REQUESTED approval override
💡 Result:
In GitHub, a pull request review with state "CHANGES_REQUESTED" blocks merging into a protected branch if branch protection rules require pull request reviews (typically requiring a minimum number of approving reviews). This is the default behavior when "Require a pull request before merging" and "Require approvals" are enabled. To override via REST API: 1. Dismiss the blocking CHANGES_REQUESTED review using the Dismiss a review endpoint (requires repo admin or dismissal permission): PUT /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id}/dismissals with body {"message": "Reason for dismissal", "event": "DISMISS"}. This changes it to a comment, removing the block (sources 1,11,12). 2. Ensure sufficient APPROVED reviews meet the required_approving_review_count. 3. Merge the PR using PUT /repos/{owner}/{repo}/pulls/{pull_number}/merge. Alternatively, admins/owners can bypass protections entirely if not restricted by "Do not allow bypassing the above settings" (sources 15,17,31). No direct API to approve overriding CHANGES_REQUESTED without dismissal; multiple approvals do not override a single CHANGES_REQUESTED if configured to block on it (source 6,31). Configure via branch protection API: PATCH /repos/{owner}/{repo}/branches/{branch}/protection/required_pull_request_reviews to set dismissal_restrictions (source 15).
Citations:
- 1: https://docs.github.com/rest/pulls/reviews
- 2: https://docs.github.com/articles/approving-a-pull-request-with-required-reviews
- 3: https://docs.github.com/en/rest/pulls/reviews
- 4: https://docs.github.com/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/dismissing-a-pull-request-review
- 5: https://docs.github.com/en/rest/branches/branch-protection
- 6: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/managing-a-branch-protection-rule
- 7: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches
🌐 Web query:
GitHub REST API PullRequestReviewThread isResolved parameter
💡 Result:
The GitHub REST API does not have a direct endpoint for PullRequestReviewThread objects or an isResolved parameter. Pull request review threads (conversation threads in PR reviews) are primarily managed via the GraphQL API, where PullRequestReviewThread includes an 'isResolved' field (boolean indicating if the thread is resolved) and mutations like resolveReviewThread exist. In REST API, you interact with reviews (/pulls/{pull_number}/reviews), review comments (/pulls/{pull_number}/comments), but no explicit thread resolution or isResolved parameter is documented in official REST endpoints as of 2026-03-10 API version. StackOverflow confirms resolution is GraphQL-only.
Citations:
- 1: https://docs.github.com/rest/pulls/reviews
- 2: https://docs.github.com/en/rest/pulls/pulls
- 3: https://stackoverflow.com/questions/71421045/how-to-resolve-a-github-pull-request-conversation-comment-thread-using-github
- 4: https://docs.github.com/en/rest/pulls/reviews
- 5: https://docs.github.com/rest/pulls/comments
🌐 Web query:
GitHub REST API pull request reviews current state vs historical
💡 Result:
In the GitHub REST API, the primary endpoint for pull request reviews is GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews, which lists all reviews for a pull request in chronological order. Each review is a distinct object (PullRequestReview) with fields including id, state (e.g., PENDING, APPROVED, CHANGES_REQUESTED, DISMISSED, COMMENTED), body, submitted_at (null for PENDING), and associated comments. Current state: This endpoint returns the current state of each review. Individual reviews can be updated via PUT /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id} (updates body), POST .../events (submits/dismisses), but the list reflects the latest state for each review ID. There is no direct support for retrieving historical states of a single review (e.g., past states before updates). Historical: The list provides a historical sequence of all reviews (multiple per user possible), ordered chronologically, allowing reconstruction of review timeline per user by sorting/filtering by user and timestamps (created_at, updated_at, submitted_at). For finer-grained history like review state changes or events, use the Issue Timeline Events endpoint GET /repos/{owner}/{repo}/issues/{issue_number}/timeline (pulls are issues), which includes 'reviewed' events with review state (approved, changes_requested, commented) and submitted_at. This captures review submissions and changes over time. No evidence of deleted reviews being retained in these endpoints. For complete audit trails, consider GitHub Audit Log API (enterprise/org level) or GraphQL API (PullRequestReviewConnection with state, submittedAt; timelineItems for events). As of 2026-03-10 API version (current docs), no dedicated review history endpoint exists in REST API; timeline provides event-based history.
Citations:
- 1: https://docs.github.com/en/rest/pulls/reviews
- 2: https://docs.github.com/rest/pulls/reviews
- 3: Parse 'reviewed' event object 'state' property google/go-github#1886
- 4: Add state and submitted_at attributes to TimelineEvent PyGithub/PyGithub#2757
REST API returns all reviews chronologically, so reviews.some() triggers on outdated CHANGES_REQUESTED even after later approval.
The REST API /pulls/{pull_number}/reviews returns the current state of each review object, but lists all reviews chronologically—multiple reviews from the same user are all included. The code checks reviews.some((r) => r.state === "CHANGES_REQUESTED"), which returns true if any review has that state, regardless of whether a later review from the same user is APPROVED. Similarly, bot comments sourced from /issues/{pull_number}/comments have no resolution tracking in the REST API; every bot comment remains treated as unresolved. The result is a sticky gate that cannot distinguish current blocking state from historical activity—clean PRs can bounce back to fix on resolved or outdated findings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/github/primitive-ops.ts` around lines 132 - 178, The code incorrectly
treats any historical CHANGES_REQUESTED as still blocking and treats all bot
comments as unresolved; fix by deduplicating reviews to only the latest review
per user (use the reviews array and group by r.user.login, keeping the item with
the newest submitted_at) and compute changesRequested from that deduped set
(replace reviews.some(...) and the CHANGES_REQUESTED filter in findings with the
deduped list), and make bot comments considered unresolved only if they are
newer than the PR’s latest update (fetch the pull via GET /pulls/{pull_number}
to get pull.updated_at and filter comments by comment.created_at >=
pull.updated_at before computing botComments and hasUnresolvedBotFindings);
update findings construction to use the deduped reviews and the filtered
botComments (references: reviewsRes, commentsRes, reviews, comments,
changesRequested, botComments, hasUnresolvedBotFindings, findings).
| export async function checkFilesChangedSince( | ||
| ctx: GitHubGateContext, | ||
| params: { pullNumber: number; pathPatterns: string }, | ||
| ): Promise<GateOpResult> { | ||
| const res = await fetch( | ||
| `https://api.github.com/repos/${ctx.owner}/${ctx.repo}/pulls/${params.pullNumber}/files?per_page=100`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }, | ||
| ); | ||
| if (!res.ok) { | ||
| return { outcome: "error", message: `GitHub API error: ${res.status}` }; | ||
| } | ||
| const files = (await res.json()) as Array<{ filename: string; status: string }>; | ||
| const patterns = params.pathPatterns.split(",").map((p) => p.trim()); | ||
| const matching = files.filter((f) => | ||
| patterns.some((pattern) => { | ||
| if (pattern.endsWith("/")) return f.filename.startsWith(pattern); | ||
| if (pattern.includes("*")) { | ||
| const regex = new RegExp(`^${pattern.replace(/\*/g, ".*")}$`); | ||
| return regex.test(f.filename); | ||
| } | ||
| return f.filename === pattern; | ||
| }), | ||
| ); | ||
| return matching.length > 0 | ||
| ? { | ||
| outcome: "changed", | ||
| message: `${matching.length} matching file(s) changed`, | ||
| artifacts: { changedFiles: matching.map((f) => f.filename) }, | ||
| } | ||
| : { outcome: "unchanged", message: "No matching files changed" }; | ||
| } |
There was a problem hiding this comment.
files_changed_since never compares against a baseline.
The op name and the docs-committed gate imply “files changed after some checkpoint,” but this implementation only inspects the PR’s current file list. If docs were edited earlier in the PR, the docs gate will pass immediately even when the technical-writer step made no new changes. Either add a sinceSha/baseSha input and diff against it, or rename the op so flow authors do not assume incremental behavior.
🧰 Tools
🪛 ast-grep (0.42.0)
[warning] 230-230: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${pattern.replace(/\*/g, ".*")}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/github/primitive-ops.ts` around lines 208 - 244, The current
checkFilesChangedSince implementation only inspects the PR's current file list
and does not compare against a baseline SHA; update checkFilesChangedSince to
accept a baseline (e.g., add params.sinceSha or params.baseSha) and use the
GitHub compare API (or commit diff between base and head) to fetch files changed
between that baseline and the PR head instead of using the pulls/files list,
then apply the existing params.pathPatterns matching logic to the compare
result; alternatively, if you prefer not to make it incremental, rename the
function and relevant exports to reflect it only checks current PR files (e.g.,
checkFilesInPR) and update callers accordingly.
|
Closing — holyship is in the platform monorepo now. |
|
Reopening — this is unmerged feature work that needs to be cherry-picked into the platform monorepo at platforms/holyship/. |
Review Summary by QodoPhase 2: Add 4 new primitive gate ops and enhance artifact extraction
WalkthroughsDescription• Add 4 new GitHub primitive gate ops for gate-driven state transitions - vcs.pr_for_branch detects PR creation by branch pattern - vcs.pr_review_status detects clean vs unresolved review findings - vcs.pr_head_changed detects fixes pushed (head SHA changed) - vcs.files_changed_since detects docs committed (matching file patterns) • Enhance existing ops to extract and return artifacts alongside outcomes - checkCommentExists extracts last matching comment body - checkPrStatus extracts PR metadata (url, number, headSha) • Engine persists gate-extracted artifacts to entity before state transition • Add outcomes maps to gates and wire artifact key mapping in primitiveOpHandler • Update @wopr-network/platform-core to 1.74.0 with cookieDomain support Diagramflowchart LR
A["GitHub API"] -->|pr_for_branch| B["vcs.pr_for_branch"]
A -->|pr_review_status| C["vcs.pr_review_status"]
A -->|pr_head_changed| D["vcs.pr_head_changed"]
A -->|files_changed_since| E["vcs.files_changed_since"]
B -->|artifacts| F["GateEvalResult"]
C -->|artifacts| F
D -->|artifacts| F
E -->|artifacts| F
F -->|resolveGate| G["Engine"]
G -->|updateArtifacts| H["Entity"]
H -->|state transition| I["Next State"]
File Changes1. tests/github/primitive-ops.test.ts
|
Code Review by Qodo
1. checkFilesChangedSince missing sinceSha
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Verified: this feature is already implemented in the platform monorepo at platforms/holyship/. All 120 tests pass. |
| /** Check if files matching path patterns were changed in a PR. */ | ||
| export async function checkFilesChangedSince( | ||
| ctx: GitHubGateContext, | ||
| params: { pullNumber: number; pathPatterns: string }, | ||
| ): Promise<GateOpResult> { | ||
| const res = await fetch( | ||
| `https://api.github.com/repos/${ctx.owner}/${ctx.repo}/pulls/${params.pullNumber}/files?per_page=100`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }, | ||
| ); | ||
| if (!res.ok) { | ||
| return { outcome: "error", message: `GitHub API error: ${res.status}` }; | ||
| } | ||
| const files = (await res.json()) as Array<{ filename: string; status: string }>; | ||
| const patterns = params.pathPatterns.split(",").map((p) => p.trim()); | ||
| const matching = files.filter((f) => | ||
| patterns.some((pattern) => { | ||
| if (pattern.endsWith("/")) return f.filename.startsWith(pattern); | ||
| if (pattern.includes("*")) { | ||
| const regex = new RegExp(`^${pattern.replace(/\*/g, ".*")}$`); | ||
| return regex.test(f.filename); | ||
| } | ||
| return f.filename === pattern; | ||
| }), | ||
| ); | ||
| return matching.length > 0 | ||
| ? { | ||
| outcome: "changed", | ||
| message: `${matching.length} matching file(s) changed`, | ||
| artifacts: { changedFiles: matching.map((f) => f.filename) }, | ||
| } | ||
| : { outcome: "unchanged", message: "No matching files changed" }; | ||
| } |
There was a problem hiding this comment.
1. checkfileschangedsince missing sincesha 📎 Requirement gap ≡ Correctness
The new vcs.files_changed_since primitive op does not accept or apply a sinceSha filter, so it cannot enforce “files changed since last state/SHA” semantics described in the committed design spec. This can cause incorrect gate outcomes (passing due to any historical docs change in the PR rather than changes since a checkpoint).
Agent Prompt
## Issue description
`vcs.files_changed_since` is implemented without `sinceSha` support and therefore cannot satisfy the design spec’s “changed since SHA” contract.
## Issue Context
The committed design spec defines `docs-committed` using `vcs.files_changed_since` with `sinceSha`, but the current implementation only inspects the PR’s full file list and uses a different param name (`pathPatterns`).
## Fix Focus Areas
- docs/specs/2026-04-01-gate-driven-signals-design.md[215-231]
- src/github/primitive-ops.ts[208-244]
- src/index.ts[372-376]
- src/flows/engineering.ts[336-350]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Persist gate-extracted artifacts (e.g., spec body, PR metadata) | ||
| if (routing.artifacts && Object.keys(routing.artifacts).length > 0) { | ||
| await entityRepo.updateArtifacts(entityId, routing.artifacts); | ||
| const refreshed = await entityRepo.get(entityId); | ||
| if (refreshed) entity = refreshed; | ||
| } |
There was a problem hiding this comment.
2. updateartifacts skips domain events 📘 Rule violation ☼ Reliability
Gate-extracted artifacts are persisted via entityRepo.updateArtifacts, but the default repo wiring uses DrizzleEntityRepository, whose updateArtifacts performs a direct DB update without emitting a domain event. This violates the requirement that event-relevant state changes be captured as domain events, making gate-driven artifact updates invisible to replay/audit consumers.
Agent Prompt
## Issue description
Gate-extracted artifact persistence uses `entityRepo.updateArtifacts`, but the default entity repo is `DrizzleEntityRepository`, which updates the DB without emitting a domain event.
## Issue Context
Compliance requires state changes (including artifact updates) to be represented as domain events so they are visible to replay/audit consumers. The PR introduces a new artifact update path specifically for gate-extracted artifacts.
## Fix Focus Areas
- src/engine/engine.ts[240-245]
- src/repositories/scoped-repos.ts[36-46]
- src/repositories/drizzle/entity.repo.ts[127-139]
- src/repositories/event-sourced/entity.repo.ts[69-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "pr-exists": { fromState: "code", trigger: "pr_created" }, | ||
| "review-status": { fromState: "review", trigger: "clean" }, | ||
| "pr-updated": { fromState: "fix", trigger: "fixes_pushed" }, | ||
| "docs-committed": { fromState: "docs", trigger: "docs_ready" }, |
There was a problem hiding this comment.
3. Gate wiring conflict 🐞 Bug ≡ Correctness
provisionEngineeringFlow() wires only the first gate matching a transition’s (fromState, trigger), but engineering.ts defines both ci-green and pr-exists for code + pr_created, so pr-exists will never be attached to any transition. This prevents gate-driven PR metadata extraction and can cause downstream gates (e.g., CI using headSha) to evaluate with missing artifacts and block routing.
Agent Prompt
## Issue description
`GATE_WIRING` defines multiple gates for the same `(fromState, trigger)` pair, but provisioning only wires a single gate (first match). As a result, `pr-exists` is never wired and cannot run.
## Issue Context
`provisionEngineeringFlow()` uses `Object.entries(GATE_WIRING).find(...)`, so only one gate can guard a transition.
## Fix Focus Areas
- src/flows/provision.ts[39-48]
- src/flows/engineering.ts[259-306]
- src/flows/engineering.ts[387-395]
## Implementation guidance
Choose one:
1) Make `GATE_WIRING` map a transition to exactly one gate (remove the duplicate wiring), OR
2) Change provisioning + flow model to support multiple gates per transition (e.g., store an array of gateIds, or create multiple transitions with priorities), OR
3) Combine PR existence + CI checks into a single primitive op/gate for `pr_created` (recommended if the model remains “one gate per transition”).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /** Check if PR head SHA differs from a known SHA (detect new pushes). */ | ||
| export async function checkPrHeadChanged( | ||
| ctx: GitHubGateContext, | ||
| params: { pullNumber: number; lastKnownSha: string }, | ||
| ): Promise<GateOpResult> { | ||
| const res = await fetch(`https://api.github.com/repos/${ctx.owner}/${ctx.repo}/pulls/${params.pullNumber}`, { | ||
| headers: { | ||
| Authorization: `Bearer ${ctx.token}`, | ||
| Accept: "application/vnd.github+json", | ||
| "X-GitHub-Api-Version": "2022-11-28", | ||
| }, | ||
| }); | ||
| if (!res.ok) { | ||
| return { outcome: "error", message: `GitHub API error: ${res.status}` }; | ||
| } | ||
| const pr = (await res.json()) as { head: { sha: string } }; | ||
| if (pr.head.sha !== params.lastKnownSha) { | ||
| return { | ||
| outcome: "changed", | ||
| message: `Head SHA changed: ${params.lastKnownSha.slice(0, 7)} → ${pr.head.sha.slice(0, 7)}`, | ||
| artifacts: { headSha: pr.head.sha }, | ||
| }; | ||
| } | ||
| return { outcome: "unchanged", message: "Head SHA has not changed" }; | ||
| } |
There was a problem hiding this comment.
4. Undefined sha crashes gate 🐞 Bug ☼ Reliability
checkPrHeadChanged() calls params.lastKnownSha.slice(...) without validating it is a string, so the gate can throw at runtime if lastKnownSha is missing/undefined. This will fail gate evaluation and can prevent the fixes_pushed transition from ever succeeding.
Agent Prompt
## Issue description
`checkPrHeadChanged` assumes `lastKnownSha` is a non-empty string and calls `.slice()` unconditionally, which can throw when the gate param is missing.
## Issue Context
Gate params are template-rendered and may be missing if prerequisite artifacts were not persisted.
## Fix Focus Areas
- src/github/primitive-ops.ts[181-205]
- src/index.ts[367-371]
- src/flows/engineering.ts[321-335]
## Implementation guidance
- Guard at the top of `checkPrHeadChanged`:
- If `typeof params.lastKnownSha !== "string" || params.lastKnownSha.length === 0`, return `{ outcome: "error", message: "Missing lastKnownSha" }` (or treat as `changed` and set `headSha`).
- Also consider validating in `primitiveOpHandler` and returning an `error` outcome if required params are missing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Summary
checkCommentExistsextracts matched comment body (last match wins)checkPrStatusextracts PR metadata (url, number, headSha)spec-postedgate maps extracted comment body toarchitectSpecartifact viaartifactKeyparamThis closes the artifact handoff gap: spec agent posts to GitHub, gate verifies AND extracts the comment body,
entity.artifacts.architectSpecis populated before the coder's prompt template renders.Implements Phase 1 of #266 (gate-driven signals).
Test plan
npx vitest run tests/github/primitive-ops.test.ts— 8 tests for comment extraction + PR metadata extractionnpx vitest run tests/engine/gate-artifact-extraction.test.ts— 3 contract tests for GateEvalResult artifactsnpx vitest run tests/flows/engineering-flow.test.ts— 29 tests including new spec-posted outcomes map testnpm run check— biome + tsc clean🤖 Generated with Claude Code
Summary by Sourcery
Add gate artifact extraction so GitHub primitive ops and gate evaluations can return and persist artifacts on entities before state transitions.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Note
Add gate artifact extraction to the engine with new GitHub primitive ops
GateEvalResultandPrimitiveOpHandlerin gate-evaluator.ts to carry an optionalartifactsfield, and updatesEngine._processSignalInnerto persist those artifacts to the entity viaentityRepo.updateArtifactsafter gate resolution.checkPrForBranch,checkPrReviewStatus,checkPrHeadChanged, andcheckFilesChangedSince, and extends existing ops (checkPrStatus,checkCommentExists) to return metadata artifacts.primitiveOpHandlerswitch in index.ts asvcs.pr_for_branch,vcs.pr_review_status,vcs.pr_head_changed, andvcs.files_changed_since, with artifact key remapping forissue_tracker.comment_exists.pr-exists,review-status,pr-updated,docs-committed) and outcome wiring for code, review, fix, and docs states.@wopr-network/platform-corefrom^1.71.0to^1.74.0.Macroscope summarized 4b6e69e.
Summary by CodeRabbit
New Features
Documentation
Tests