diff --git a/.claude/skills/portos-file-issue/SKILL.md b/.claude/skills/portos-file-issue/SKILL.md index fcfd9336c1..42d88d1c4e 100644 --- a/.claude/skills/portos-file-issue/SKILL.md +++ b/.claude/skills/portos-file-issue/SKILL.md @@ -16,5 +16,5 @@ description: PortOS conventions for filing GitHub issues and capturing deferred - `help wanted` — extra hands welcome and the body is scoped enough to pick up cold. - **Planner attribution (`planner:`) records who WROTE the plan.** It is a third independent axis — the author, not a recommendation about how to run the work. Take the label verbatim from the "Planner Attribution" section of your own run's prompt (PortOS resolves it from the provider/model it dispatched you with); **never guess it from what you believe you are**, and omit the axis entirely when your run was given none. Create it like any other label: `gh label create planner: --color C2185B --description 'Plan authored by the model' 2>/dev/null || true`. Omit an axis rather than guessing. Do not stamp `medium` on both dispatch axes. Do not put `[model:…]` / `[effort:…]` / `[category]` / `[SEVERITY]` in the title. Create each missing label immediately before applying it (`gh label create --color 2>/dev/null || true`; colors: model:light `D4C5F9`, model:medium `A371F7`, model:heavy `6F42C1`, effort:low `BFE5E5`, effort:medium `76C7C7`, effort:high `1D7874`, effort:xhigh `0E4F4C`, effort:max `05403D`, good first issue `7057FF`, help wanted `008672`). Use repeated `--label` flags. Never relabel a deduplicated existing issue. - - **`future` is almost never correct.** Do NOT use it to park a choice you could make yourself. The narrow legitimate park is work a **human must personally drive** — a real fidelity/validation run only they can judge, or a step that needs **specific hardware/credentials** the agent doesn't have (e.g. a CUDA box, an Apple-Silicon-only validation). Mark those `blocked` (skipped by autonomous claim runs), not `future`, and say in the body exactly what human action unblocks it. `blocked` also covers a genuine dependency on another unshipped issue/PR. + - **`future` is almost never correct.** Do NOT use it to park a choice you could make yourself. The narrow legitimate park is work a **human must personally drive** — a real fidelity/validation run only they can judge, or a step that needs **specific hardware/credentials** the agent doesn't have (e.g. a CUDA box, an Apple-Silicon-only validation). Mark those `blocked` (skipped by autonomous claim runs), not `future`, and say in the body exactly what human action unblocks it. `blocked` also covers a genuine dependency on another unshipped issue/PR — for that case, name the dependency on its own line as `Blocked by #N` (or `Blocked by #N, #M` for more than one): the `issue-reconcile` scheduled task (`blockedIssueReconcile.js`) parses exactly that line, and removes the label automatically once every named issue is closed. An issue with no `Blocked by` line is never auto-unblocked, so the human/hardware case is unaffected. - **`needs-input` is a last resort, not a reflex.** When claiming an ambiguous issue, prefer proceeding on your best interpretation and shipping it. Fall back to `needs-input` only when proceeding would be **destructive/irreversible** or genuinely requires the human (hardware, credentials, a personal judgment call) — never merely because more than one reasonable reading exists. diff --git a/server/lib/README.md b/server/lib/README.md index c788dddbf7..f49bc6e012 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -251,6 +251,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `createKeyCachedQueue.js` | Per-KEY serialized async work queue (sibling to `fileWriteQueue.js`'s single tail). `createKeyCachedQueue()` returns `queue(key, work)` that chains each `work` thunk onto the prior in-flight promise for that `key` — same-key work runs one-after-another (later sees earlier's committed result), different keys run concurrently. Self-pruning tail Map; `work` runs on both fulfil and reject so one failure can't stall the chain; carries `.clear()` for test reset. Used by the media-job completion hooks (writers-room / catalog / music-video scene-image attach) to serialize per-record. | | `createNewestWinsGuard.js` | Newest-render-wins ordering guard for out-of-order async completions. `createNewestWinsGuard()` returns `{ isStale(key, at), mark(key, at), clear() }` — tracks the newest applied `queuedAt` per slot `key` so an older render completing after a newer one is dropped (`isStale` true) instead of clobbering the newer frame. ISO timestamps compare as strings; absent `at` is never stale. Used by `createMediaJobImageHook`'s opt-in guard and the catalog hook's portrait slot. | | `fileWriteQueue.js` | Serialize read-modify-write cycles so they don't interleave. `createFileWriteQueue()` → single-tail `queue(fn)` for one shared file; `createRecordWriteQueue(assertId?)` → id-keyed `queueRecordWrite(id, fn)` where same-id cycles serialize and different-id cycles run in parallel (the queue the PG/file store facades use for `collectionStore.queueRecordWrite` parity); `createKeyedFileWriteQueue()` → `queueKeyedWrite(key, fn)` that collapses the pipeline stage stores' hand-rolled `Map` + per-key `createFileWriteQueue` factory (falsy keys → shared `'__unknown__'` tail; self-pruning via `createKeyCachedQueue`). | +| `forgeIssueState.js` | `normalizeIssueState(state)` — collapse a forge issue/PR/MR state string to `open`/`closed`. GitHub reports `OPEN`/`CLOSED`; GitLab reports `opened`/`closed`/`locked`; anything unrecognized is treated as open so an unfamiliar state can never read as "already resolved". Shared by `layeredIntelligence/forgeFiler.js` and `services/blockedIssueReconcile.js`. | | `imageClean.js` | `cleanImageBuffer(buf, { metadata, denoise })` (composable opt-in pipeline: lossless metadata/C2PA strip + optional median/sharpen denoise) · `stripPngMetadataChunks` / `stripPngC2PAChunk` (lossless PNG-chunk removers) · `compositeIgnoreZone(base, original, mask, { feather })` (preserve-region compositing: restore original pixels into a feathered mask over a diffused result) · `autoCleanGeneratedImage` (in-place clean for post-generation hook). HTTP route in `routes/imageClean.js` wraps `cleanImageBuffer` and appends a CPU light diffusion pass (`applyLightRegen` from `services/imageGen/regen.js`) for the `diffusion=light` SynthID-disruption step. | | `imageFrameStats.js` | Degenerate-frame classifier (#4173) — `describeFrameStats(bufferOrPath)` runs one `sharp` `.stats()` decode and returns `{ ok, reason, perChannel }`, rejecting `solid-fill` (every colour channel stdev under `SOLID_FILL_STDEV_EPSILON`), `fully-transparent` (alpha max 0) and `near-empty` (greyscale entropy under `NEAR_EMPTY_ENTROPY_FLOOR`), except when a non-opaque alpha channel carries a substantial silhouette itself. Deliberately NOT a quality judge: a legitimately dark or minimalist render keeps real per-channel variance and is accepted. `ok` is three-valued — `true`/`false`/`null`, where `null` means could-not-measure (undecodable buffer, or under `MIN_JUDGEABLE_PIXELS`) and must never read as degenerate; gate on `isDegenerateFrame(stats)` / `ok === false`, never `!ok`. Called by the Image Gen provider completion seams, sprite reference normalization, and `visionTest.js` (so a paid vision call is never spent on a blank frame). A **buffer** verdict is memoized on the sha256 of its bytes (#6004) — the probe costs ~4ms regardless of pixel count and the sprite compiler re-probes identical frames dozens of times per run; a **path** is never memoized (the file behind it can change), so hand it bytes you already read. `stats-unavailable` is never memoized either — that branch catches transient probe failures too, and pinning `ok: null` on valid bytes would disable the gate for them. `__resetFrameStatsCache()` drops the memo in tests. | | `imageRgba.js` | Sharp-backed RGBA boundary: `decodeRgbaFrame` decodes an image to `{ data, width, height }`, and `encodePng` turns a raw frame into PNG bytes for the caller's destination/hash policy. | diff --git a/server/lib/forgeIssueState.js b/server/lib/forgeIssueState.js new file mode 100644 index 0000000000..e424c6d844 --- /dev/null +++ b/server/lib/forgeIssueState.js @@ -0,0 +1,20 @@ +/** + * Normalize a forge issue/PR/MR state string to `open` / `closed`. GitLab + * reports `opened` (and `closed`/`locked`); GitHub reports `open`/`closed`. + * Anything unrecognized is treated as open, so an unfamiliar state string + * never reads as "already resolved" and short-circuits a scan that depends on + * catching every still-open item (dedup/park/blocker checks). (`merged` never + * applies to issues, only PRs/MRs.) + * + * Shared by `layeredIntelligence/forgeFiler.js` and `blockedIssueReconcile.js` + * — lifted here (rather than duplicated, or imported cross-subsystem from + * `layeredIntelligence/`) per the Import scoping convention in + * `server/AGENTS.md`. + * @param {string} state + * @returns {'open'|'closed'} + */ +export function normalizeIssueState(state) { + const s = (state || '').toLowerCase(); + if (s === 'closed' || s === 'locked') return 'closed'; + return 'open'; +} diff --git a/server/lib/index.js b/server/lib/index.js index f4e83b1fcd..7a9b7d0508 100644 --- a/server/lib/index.js +++ b/server/lib/index.js @@ -226,6 +226,7 @@ export * from './agentInstructionsFile.js'; export * from './fileCore.js'; export * as fileUtils from './fileUtils.js'; export * from './fileWriteQueue.js'; +export * from './forgeIssueState.js'; export * from './portosEnv.js'; export * from './homePath.js'; export * from './jsonIo.js'; diff --git a/server/services/blockedIssueReconcile.js b/server/services/blockedIssueReconcile.js new file mode 100644 index 0000000000..32a8887ece --- /dev/null +++ b/server/services/blockedIssueReconcile.js @@ -0,0 +1,230 @@ +/** + * Blocked-Issue Reconciler — deterministic core. + * + * The `blocked` label (portos-file-issue skill) covers two different reasons an + * issue is parked: a step only a human can drive (hardware/credentials), or "a + * genuine dependency on another unshipped issue/PR." Nothing removes the label + * once the second case resolves — the claim queue (`NON_ACTIONABLE_ISSUE_LABELS` + * in perpetualWork.js) skips every `blocked` issue regardless of *why* it is + * blocked, so a dependency-blocked issue sits parked forever even after its + * blocker ships. + * + * This scan finds open `blocked` issues whose body names its blocker(s) via a + * `Blocked by #N[, #M ...]` line, checks whether every named blocker is closed, + * and — only when ALL of them are — removes the `blocked` label and posts a + * comment. An issue with no parsed blocker reference is left untouched: that is + * the human/hardware case, and this scan draws no inference from silence. + * + * Forge-agnostic like issueReconcile.js: GitHub via `gh`, GitLab via `glab`, + * resolved from the git origin host through `resolveRepoForgeTarget` / + * `resolveAppForgeTarget`. The decision needs no judgment (a blocker is either + * closed or it isn't), so — mirroring `releaseAbandonedClaims` in + * issueReconcile.js — this is a pure deterministic write with no coordinator + * agent and no LLM call. + */ + +import { execGh, ensureForgeReachable } from './github.js'; +import { execGlab, execGlabJson } from './gitlab.js'; +import { resolveAppForgeTarget, resolveRepoForgeTarget } from '../lib/workTracker.js'; +import { safeJSONParse } from '../lib/fileUtils.js'; +import { normalizeIssueState } from '../lib/forgeIssueState.js'; + +export const BLOCKED_LABEL = 'blocked'; + +// Bound the forge queries — single-user repos never realistically truncate at +// this size (mirrors issueReconcile.js's GH_LIST_LIMIT / GL_PER_PAGE). +const GH_LIST_LIMIT = 100; +const GH_ALL_STATE_LIMIT = 300; +const GL_PER_PAGE = 100; + +/** + * Parse the issue numbers named by a `Blocked by #N[, #M, ...]` line (an + * optional leading bullet/dash and an optional trailing colon are tolerated), + * case-insensitive. Returns a sorted, deduped array of positive integers, or + * `[]` when the body names no blocker — that empty result is the signal this + * scan leaves the issue alone; a human/hardware block has no dependency to + * resolve, so absence of the trailer must never be read as "ready to unblock." + * + * Deliberately narrow: only text on a `Blocked by` line counts, not any `#N` + * mention anywhere in the body — an ordinary cross-reference, `Related: #N`, or + * `Refs #N` elsewhere in the issue must never be treated as a dependency this + * scan can clear. + * @param {string} body + * @returns {number[]} + */ +export function parseBlockingIssueNumbers(body) { + if (!body) return []; + const nums = new Set(); + const lineRe = /^[ \t*-]*Blocked by:?\s*(.*)/gim; + let line; + while ((line = lineRe.exec(body))) { + const numRe = /#(\d+)/g; + let n; + while ((n = numRe.exec(line[1]))) nums.add(Number(n[1])); + } + return [...nums].sort((a, b) => a - b); +} + +/** + * Fetch GitHub's open `blocked` issues plus a `number → state` map covering + * every issue (open + closed, bounded by GH_ALL_STATE_LIMIT) so each blocker + * reference can be resolved from one extra call rather than one `gh issue view` + * per blocker. Returns null on any gh failure — the caller treats null as + * "skip this cycle", never as "no blocked issues" or "every blocker is open". + * @returns {Promise<{ blocked: object[], stateByNumber: Map }|null>} + */ +async function getGithubBlockedState(repoSpec, apiHost) { + const forge = await ensureForgeReachable('blocked-issue-reconcile', { hostname: apiHost }); + if (!forge.ok) return null; + + const ghList = (args, what) => execGh(args).catch((err) => { + console.error(`❌ blocked-issue-reconcile: ${what} failed for ${repoSpec}: ${err.message}`); + return null; + }); + + const [blockedRaw, allRaw] = await Promise.all([ + ghList(['issue', 'list', '--repo', repoSpec, '--state', 'open', + '--label', BLOCKED_LABEL, '--limit', String(GH_LIST_LIMIT), + '--json', 'number,title,body,url'], 'gh issue list --label blocked'), + ghList(['issue', 'list', '--repo', repoSpec, '--state', 'all', + '--limit', String(GH_ALL_STATE_LIMIT), '--json', 'number,state'], 'gh issue list --state all'), + ]); + + const blocked = safeJSONParse(blockedRaw, null); + if (!Array.isArray(blocked)) return null; + // The blocked list can legitimately be empty (nothing to unblock) — a caller + // reads that as "no candidates", not "gh failed", because it parsed fine. + const all = safeJSONParse(allRaw, null); + if (!Array.isArray(all)) return null; + + const stateByNumber = new Map(); + for (const issue of all) { + if (Number.isInteger(issue?.number)) stateByNumber.set(issue.number, normalizeIssueState(issue.state)); + } + return { + blocked: blocked.map((i) => ({ number: i.number, title: i.title || '', url: i.url || '', body: i.body || '' })), + stateByNumber, + }; +} + +/** + * GitLab mirror of `getGithubBlockedState`. `glab issue list --all` returns + * every issue's `iid` + `state` in one call, avoiding a per-blocker view call. + * @returns {Promise<{ blocked: object[], stateByNumber: Map }|null>} + */ +async function getGitlabBlockedState(repoPath) { + const [blocked, all] = await Promise.all([ + execGlabJson(['issue', 'list', '--label', BLOCKED_LABEL, '--per-page', String(GL_PER_PAGE)], repoPath), + execGlabJson(['issue', 'list', '--all', '--per-page', String(GL_PER_PAGE)], repoPath), + ]); + if (!blocked.rows || !all.rows) { + console.error(`❌ blocked-issue-reconcile: glab issue list unavailable (${blocked.reason}/${all.reason}) — skipping this cycle`); + return null; + } + const stateByNumber = new Map(); + for (const issue of all.rows) { + if (Number.isInteger(issue?.iid)) stateByNumber.set(issue.iid, normalizeIssueState(issue.state)); + } + return { + blocked: blocked.rows.map((i) => ({ + number: i.iid, title: i.title || '', url: i.web_url || '', body: i.description || '', + })), + stateByNumber, + }; +} + +/** + * Gather + classify: for every open `blocked` issue that names blocker(s) via + * the `Blocked by #N` convention, check whether all of them are closed. Pure + * classification over already-fetched state. + * @param {object[]} blocked + * @param {Map} stateByNumber + * @returns {object[]} entries with `blockingNumbers`, `closedBlockers`, `openBlockers` — only issues with at least one parsed blocker are included + */ +export function classifyBlockedIssues(blocked, stateByNumber) { + return blocked + .map((issue) => ({ ...issue, blockingNumbers: parseBlockingIssueNumbers(issue.body) })) + .filter((issue) => issue.blockingNumbers.length > 0) + .map((issue) => { + const closedBlockers = issue.blockingNumbers.filter((n) => stateByNumber.get(n) === 'closed'); + // A blocker whose state could not be resolved (absent from the map) must + // NOT read as closed — it stays in openBlockers, same absent-vs-false + // discipline as issueReconcile's hasForeignClaim. + const openBlockers = issue.blockingNumbers.filter((n) => stateByNumber.get(n) !== 'closed'); + return { number: issue.number, title: issue.title, url: issue.url, blockingNumbers: issue.blockingNumbers, closedBlockers, openBlockers }; + }); +} + +/** + * Full gather + classify for one app's forge repo. Returns `{ forge, repoSpec, + * fullName, ready }` where `ready` is every dependency-blocked issue whose + * blockers are ALL closed — or null on an unsupported remote / transient + * failure, so the caller skips this cycle without treating it as "nothing to + * unblock". + * @param {string} repoPath + * @param {{ app?: object }} [opts] + */ +export async function gatherBlockedIssueState(repoPath, { app = null } = {}) { + const target = app + ? (await resolveAppForgeTarget(app, { repoPath })).target + : await resolveRepoForgeTarget(repoPath); + if (!target) return null; + + let state = null; + if (target.forge === 'github') state = await getGithubBlockedState(target.repoSpec, target.apiHost); + else if (target.forge === 'gitlab') state = await getGitlabBlockedState(repoPath); + if (!state) return null; + + return { + forge: target.forge, + repoSpec: target.repoSpec ?? null, + fullName: target.fullName, + ready: classifyBlockedIssues(state.blocked, state.stateByNumber).filter((i) => i.openBlockers.length === 0), + }; +} + +/** + * Remove the `blocked` label (and post an explanatory comment first) on every + * issue whose dependency is now fully resolved. The comment is posted BEFORE + * the label removal so an unblock is never silent, mirroring + * `releaseAbandonedClaims`; a failed comment does not block the removal. + * @param {object[]} ready - from `gatherBlockedIssueState().ready` + * @param {{ forge:string, repoSpec:string|null, fullName:string, repoPath:string }} ctx + * @returns {Promise} how many issues were actually unblocked + */ +export async function unblockIssues(ready, { forge, repoSpec, fullName, repoPath }) { + if (!ready?.length) return 0; + if (forge !== 'github' && forge !== 'gitlab') return 0; + if (forge === 'github' && !repoSpec) return 0; + + let unblocked = 0; + for (const issue of ready) { + const number = String(issue.number); + const blockersList = issue.closedBlockers.map((n) => `#${n}`).join(', '); + const commentBody = `Unblocking: every issue named in \`Blocked by\` (${blockersList}) is now closed. Removing the \`${BLOCKED_LABEL}\` label so this rejoins the claim queue.`; + + let ok; + if (forge === 'github') { + await execGh(['issue', 'comment', number, '--repo', repoSpec, '--body', commentBody]).catch((err) => { + console.error(`❌ blocked-issue-reconcile: could not comment on #${number} in ${fullName}: ${err.message}`); + }); + ok = await execGh(['issue', 'edit', number, '--repo', repoSpec, '--remove-label', BLOCKED_LABEL]) + .then(() => true) + .catch((err) => { + console.error(`❌ blocked-issue-reconcile: could not unblock #${number} in ${fullName}: ${err.message}`); + return false; + }); + } else { + const noted = await execGlab(['issue', 'note', number, '--message', commentBody], repoPath); + if (noted === null) console.error(`❌ blocked-issue-reconcile: could not comment on #${number} in ${fullName}`); + const result = await execGlab(['issue', 'update', number, '--unlabel', BLOCKED_LABEL], repoPath); + ok = result !== null; + if (!ok) console.error(`❌ blocked-issue-reconcile: could not unblock #${number} in ${fullName}`); + } + if (ok) { + unblocked += 1; + console.log(`🔓 blocked-issue-reconcile unblocked #${number} in ${fullName}: ${blockersList} closed`); + } + } + return unblocked; +} diff --git a/server/services/blockedIssueReconcile.test.js b/server/services/blockedIssueReconcile.test.js new file mode 100644 index 0000000000..4c8d604b7f --- /dev/null +++ b/server/services/blockedIssueReconcile.test.js @@ -0,0 +1,233 @@ +/** + * Unit tests for the Blocked-Issue Reconciler deterministic core. + * + * - parseBlockingIssueNumbers — the `Blocked by #N` line parser, narrow on + * purpose (must not pick up an unrelated `#N` mention elsewhere in the body). + * - classifyBlockedIssues — pure "are ALL named blockers closed" classifier, + * with the absent-state-must-not-read-as-closed discipline. + * - gatherBlockedIssueState — end-to-end over mocked gh/glab. + * - unblockIssues — the one WRITE this module performs: comment-then-unlabel, + * on both forges, with failure handling. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const ensureForgeReachableMock = vi.fn(async () => ({ ok: true, status: 'ok', detail: null, remedy: null })); +const execGhMock = vi.fn(async () => '[]'); +vi.mock('./github.js', () => ({ + execGh: (...args) => execGhMock(...args), + ensureForgeReachable: (...args) => ensureForgeReachableMock(...args), +})); +const execGlabMock = vi.fn(async () => 'ok'); +const execGlabJsonMock = vi.fn(async () => ({ rows: [], reason: 'ok' })); +vi.mock('./gitlab.js', () => ({ + execGlab: (...args) => execGlabMock(...args), + execGlabJson: (...args) => execGlabJsonMock(...args), +})); +vi.mock('../lib/gitRemote.js', () => ({ + getOriginInfo: vi.fn(async () => ({ isGithub: true, host: 'github.com', fullName: 'atomantic/PortOS' })), + readOriginRemoteUrl: vi.fn(async () => 'git@github.com:atomantic/PortOS.git'), +})); +// workTracker.js is intentionally NOT mocked — same rationale as +// issueReconcile.test.js: it is the canonical origin→forge classifier and its +// only effectful dependency (gitRemote.js) is mocked above. +vi.mock('../lib/fileUtils.js', () => ({ + safeJSONParse: (raw, fallback) => { try { return JSON.parse(raw); } catch { return fallback; } }, +})); + +import { + parseBlockingIssueNumbers, classifyBlockedIssues, gatherBlockedIssueState, unblockIssues, BLOCKED_LABEL, +} from './blockedIssueReconcile.js'; +import { getOriginInfo, readOriginRemoteUrl } from '../lib/gitRemote.js'; + +beforeEach(() => { + vi.clearAllMocks(); + ensureForgeReachableMock.mockResolvedValue({ ok: true, status: 'ok', detail: null, remedy: null }); + execGhMock.mockResolvedValue('[]'); + execGlabMock.mockResolvedValue('ok'); + execGlabJsonMock.mockResolvedValue({ rows: [], reason: 'ok' }); + getOriginInfo.mockResolvedValue({ isGithub: true, host: 'github.com', fullName: 'atomantic/PortOS' }); + readOriginRemoteUrl.mockResolvedValue('git@github.com:atomantic/PortOS.git'); +}); + +describe('parseBlockingIssueNumbers', () => { + it('parses a single blocker', () => { + expect(parseBlockingIssueNumbers('Some text\nBlocked by #123\nmore text')).toEqual([123]); + }); + it('parses multiple blockers on one line, comma or "and" separated', () => { + expect(parseBlockingIssueNumbers('Blocked by #123, #456 and #789')).toEqual([123, 456, 789]); + }); + it('is case-insensitive and tolerates a trailing colon', () => { + expect(parseBlockingIssueNumbers('blocked BY: #42')).toEqual([42]); + }); + it('tolerates a leading bullet/dash', () => { + expect(parseBlockingIssueNumbers('- Blocked by #7')).toEqual([7]); + }); + it('dedupes and sorts', () => { + expect(parseBlockingIssueNumbers('Blocked by #9\nBlocked by #9, #3')).toEqual([3, 9]); + }); + it('does NOT match an unrelated #N mention elsewhere in the body', () => { + expect(parseBlockingIssueNumbers('See #123 for background.\nRelated: #456')).toEqual([]); + }); + it('does NOT match "Refs #N" or "Related: #N" as a dependency', () => { + expect(parseBlockingIssueNumbers('Refs #99\nRelated: #100')).toEqual([]); + }); + it('returns [] for a body with no Blocked by line', () => { + expect(parseBlockingIssueNumbers('Just a plain description.')).toEqual([]); + }); + it('returns [] for an absent/empty body', () => { + expect(parseBlockingIssueNumbers('')).toEqual([]); + expect(parseBlockingIssueNumbers(undefined)).toEqual([]); + }); +}); + +describe('classifyBlockedIssues', () => { + it('marks an issue ready when its single blocker is closed', () => { + const [result] = classifyBlockedIssues( + [{ number: 1, title: 'A', url: '', body: 'Blocked by #10' }], + new Map([[10, 'closed']]) + ); + expect(result.openBlockers).toEqual([]); + expect(result.closedBlockers).toEqual([10]); + }); + it('keeps an issue blocked when one of several blockers is still open', () => { + const [result] = classifyBlockedIssues( + [{ number: 1, title: 'A', url: '', body: 'Blocked by #10, #11' }], + new Map([[10, 'closed'], [11, 'open']]) + ); + expect(result.openBlockers).toEqual([11]); + expect(result.closedBlockers).toEqual([10]); + }); + it('treats a blocker with unresolved state as still open (absent must not read as closed)', () => { + const [result] = classifyBlockedIssues( + [{ number: 1, title: 'A', url: '', body: 'Blocked by #10' }], + new Map() // #10's state could not be fetched + ); + expect(result.openBlockers).toEqual([10]); + }); + it('excludes an issue with no parsed blocker (the human/hardware case)', () => { + const result = classifyBlockedIssues( + [{ number: 1, title: 'A', url: '', body: 'Waiting on hardware.' }], + new Map() + ); + expect(result).toEqual([]); + }); +}); + +describe('gatherBlockedIssueState (GitHub)', () => { + it('returns ready=[] when the blocked list is empty', async () => { + execGhMock.mockResolvedValueOnce('[]').mockResolvedValueOnce('[]'); + const result = await gatherBlockedIssueState('/repo'); + expect(result.forge).toBe('github'); + expect(result.ready).toEqual([]); + }); + + it('resolves a blocked issue whose named blocker is now closed', async () => { + execGhMock + .mockResolvedValueOnce(JSON.stringify([{ number: 5, title: 'Feature X', body: 'Blocked by #10', url: 'u' }])) + .mockResolvedValueOnce(JSON.stringify([{ number: 10, state: 'CLOSED' }, { number: 5, state: 'OPEN' }])); + const result = await gatherBlockedIssueState('/repo'); + expect(result.ready).toEqual([ + expect.objectContaining({ number: 5, blockingNumbers: [10], closedBlockers: [10], openBlockers: [] }), + ]); + }); + + it('leaves a blocked issue out of ready when its blocker is still open', async () => { + execGhMock + .mockResolvedValueOnce(JSON.stringify([{ number: 5, title: 'Feature X', body: 'Blocked by #10', url: 'u' }])) + .mockResolvedValueOnce(JSON.stringify([{ number: 10, state: 'OPEN' }])); + const result = await gatherBlockedIssueState('/repo'); + expect(result.ready).toEqual([]); + }); + + it('returns null (skip, not "nothing to unblock") when gh is unreachable', async () => { + ensureForgeReachableMock.mockResolvedValueOnce({ ok: false, status: 'error', detail: 'offline', remedy: null }); + const result = await gatherBlockedIssueState('/repo'); + expect(result).toBeNull(); + }); + + it('returns null when the blocked-issue list call fails', async () => { + execGhMock.mockRejectedValueOnce(new Error('gh blip')); + const result = await gatherBlockedIssueState('/repo'); + expect(result).toBeNull(); + }); +}); + +describe('gatherBlockedIssueState (GitLab)', () => { + beforeEach(() => { + getOriginInfo.mockResolvedValue({ isGithub: false, host: 'gitlab.com', fullName: 'group/proj' }); + readOriginRemoteUrl.mockResolvedValue('git@gitlab.com:group/proj.git'); + }); + + it('resolves a blocked issue whose named blocker is now closed', async () => { + execGlabJsonMock + .mockResolvedValueOnce({ rows: [{ iid: 5, title: 'Feature X', description: 'Blocked by #10', web_url: 'u' }], reason: 'ok' }) + .mockResolvedValueOnce({ rows: [{ iid: 10, state: 'closed' }, { iid: 5, state: 'opened' }], reason: 'ok' }); + const result = await gatherBlockedIssueState('/repo'); + expect(result.forge).toBe('gitlab'); + expect(result.ready).toEqual([ + expect.objectContaining({ number: 5, closedBlockers: [10], openBlockers: [] }), + ]); + }); + + it('returns null when glab cannot answer', async () => { + execGlabJsonMock.mockResolvedValue({ rows: null, reason: 'cli-failed' }); + const result = await gatherBlockedIssueState('/repo'); + expect(result).toBeNull(); + }); +}); + +describe('unblockIssues (GitHub)', () => { + const ready = [{ number: 5, title: 'Feature X', url: 'u', blockingNumbers: [10], closedBlockers: [10], openBlockers: [] }]; + const ctx = { forge: 'github', repoSpec: 'github.com/o/r', fullName: 'o/r', repoPath: '/repo' }; + + it('comments then removes the label, and counts one unblock', async () => { + execGhMock.mockResolvedValue(''); + const count = await unblockIssues(ready, ctx); + expect(count).toBe(1); + const calls = execGhMock.mock.calls.map((c) => c[0]); + expect(calls[0]).toEqual(['issue', 'comment', '5', '--repo', 'github.com/o/r', '--body', expect.stringContaining('#10')]); + expect(calls[1]).toEqual(['issue', 'edit', '5', '--repo', 'github.com/o/r', '--remove-label', BLOCKED_LABEL]); + }); + + it('does not count a failed label removal, even if the comment succeeded', async () => { + execGhMock + .mockResolvedValueOnce('') // comment succeeds + .mockRejectedValueOnce(new Error('422')); // edit fails + const count = await unblockIssues(ready, ctx); + expect(count).toBe(0); + }); + + it('still attempts the label removal when the comment call fails', async () => { + execGhMock + .mockRejectedValueOnce(new Error('comment failed')) + .mockResolvedValueOnce(''); + const count = await unblockIssues(ready, ctx); + expect(count).toBe(1); + }); + + it('returns 0 for an empty ready list without calling gh', async () => { + const count = await unblockIssues([], ctx); + expect(count).toBe(0); + expect(execGhMock).not.toHaveBeenCalled(); + }); +}); + +describe('unblockIssues (GitLab)', () => { + const ready = [{ number: 5, title: 'Feature X', url: 'u', blockingNumbers: [10], closedBlockers: [10], openBlockers: [] }]; + const ctx = { forge: 'gitlab', repoSpec: null, fullName: 'group/proj', repoPath: '/repo' }; + + it('notes then unlabels via glab, and counts one unblock', async () => { + const count = await unblockIssues(ready, ctx); + expect(count).toBe(1); + const calls = execGlabMock.mock.calls.map((c) => c[0]); + expect(calls[0]).toEqual(['issue', 'note', '5', '--message', expect.stringContaining('#10')]); + expect(calls[1]).toEqual(['issue', 'update', '5', '--unlabel', BLOCKED_LABEL]); + }); + + it('does not count a failed unlabel call', async () => { + execGlabMock.mockResolvedValueOnce('ok').mockResolvedValueOnce(null); + const count = await unblockIssues(ready, ctx); + expect(count).toBe(0); + }); +}); diff --git a/server/services/cosTaskPreStepBlocks.js b/server/services/cosTaskPreStepBlocks.js index ac35a9a3ec..10efed0f46 100644 --- a/server/services/cosTaskPreStepBlocks.js +++ b/server/services/cosTaskPreStepBlocks.js @@ -568,6 +568,28 @@ export async function resolveIssueReconcileBlock(app, taskType, metadata, taskSc if (releasedCount) { emitLog('info', `🔓 issue-reconcile ${app.name}: released ${releasedCount} abandoned claim(s) back to the queue`, { appId: app.id, analysisType: taskType }); } + // Also deterministic, also no model needed: an issue labeled `blocked` on a + // genuine dependency (`Blocked by #N` convention — portos-file-issue skill) + // whose blocker(s) have all since closed is unlabeled here, not gated behind + // the zombie coordinator. gatherBlockedIssueState resolves its own forge + // target and returns null for anything but GitHub/GitLab (JIRA included — + // it has no equivalent scan), so no forge pre-check is needed here. + const { gatherBlockedIssueState, unblockIssues } = await import('./blockedIssueReconcile.js'); + const blockedState = await gatherBlockedIssueState(app.repoPath, { app }).catch((err) => { + emitLog('warn', `issue-reconcile could not scan blocked issues for ${app.name}: ${err.message}`, { appId: app.id }); + return null; + }); + if (blockedState?.ready.length) { + const unblockedCount = await unblockIssues(blockedState.ready, { + forge: blockedState.forge, repoSpec: blockedState.repoSpec, fullName: blockedState.fullName, repoPath: app.repoPath, + }).catch((err) => { + emitLog('warn', `issue-reconcile could not unblock issues for ${app.name}: ${err.message}`, { appId: app.id }); + return 0; + }); + if (unblockedCount) { + emitLog('info', `🔓 issue-reconcile ${app.name}: removed the \`blocked\` label from ${unblockedCount} issue(s) whose dependency closed`, { appId: app.id, analysisType: taskType }); + } + } if (result.stalled.length) { // In-progress issues with NO merged PR and NO live claim — a different stuck // state issue-reconcile deliberately does NOT auto-heal. Surface them. diff --git a/server/services/gitlab.js b/server/services/gitlab.js index 2e525d7a53..ef812c8ec5 100644 --- a/server/services/gitlab.js +++ b/server/services/gitlab.js @@ -18,16 +18,17 @@ const DEFAULT_EXEC_GLAB_TIMEOUT_MS = 60000; * — callers treat null as "unavailable / transient", mirroring the * `.catch(() => null)` pattern used around `execGh`. * - * Module-local on purpose: every PortOS caller wants JSON, so they go through - * `execGlabJson`, which owns the output flag (see lib/glabArgs.js). Export this - * only when a genuine non-JSON `glab` call appears. + * Most PortOS callers want JSON and go through `execGlabJson`, which owns the + * output flag (see lib/glabArgs.js). This is exported for the genuine non-JSON + * `glab` calls — mutations like `issue update --unlabel` / `issue note`, which + * return human text on success (see `blockedIssueReconcile.js`). * * @param {string[]} args - glab arguments (e.g. ['issue', 'list', '--output', 'json']) * @param {string} cwd - repo root the glab command runs in * @param {number} [timeoutMs] - kills the child and resolves null past this * @returns {Promise} */ -function execGlab(args, cwd, timeoutMs = DEFAULT_EXEC_GLAB_TIMEOUT_MS) { +export function execGlab(args, cwd, timeoutMs = DEFAULT_EXEC_GLAB_TIMEOUT_MS) { return new Promise((resolve) => { const child = spawn('glab', args, { cwd, shell: false }); let stdout = ''; diff --git a/server/services/layeredIntelligence/forgeFiler.js b/server/services/layeredIntelligence/forgeFiler.js index 6821dc7eeb..e5de7b9b01 100644 --- a/server/services/layeredIntelligence/forgeFiler.js +++ b/server/services/layeredIntelligence/forgeFiler.js @@ -7,22 +7,13 @@ import { dispatchLabelSpec, forgeIssueLabels } from '../../lib/dispatchLabels.js'; import { safeJSONParse } from '../../lib/fileUtils.js'; +import { normalizeIssueState } from '../../lib/forgeIssueState.js'; import { LI_LABEL, LI_BLOCKING_LABEL } from './constants.js'; import { slugMarker, extractSlugFromBody } from './dedup.js'; import { runCli } from './runCli.js'; import { withGlabJson } from '../../lib/glabArgs.js'; -/** - * Normalize a forge issue state to `open` / `closed`. GitLab reports `opened` - * (and `closed`/`locked`); GitHub reports `open`/`closed`. Anything that isn't a - * recognized closed/locked state is treated as open so dedup + park don't miss a - * GitLab-`opened` issue. (`merged` never applies to issues.) - */ -export function normalizeIssueState(state) { - const s = (state || '').toLowerCase(); - if (s === 'closed' || s === 'locked') return 'closed'; - return 'open'; -} +export { normalizeIssueState }; /** * Normalize a forge issue's labels to a plain `string[]`. gh reports objects diff --git a/server/services/taskScheduleRegistry.js b/server/services/taskScheduleRegistry.js index 84cb90f1cf..7355a7063f 100644 --- a/server/services/taskScheduleRegistry.js +++ b/server/services/taskScheduleRegistry.js @@ -310,6 +310,12 @@ export const DEFAULT_TASK_INTERVALS = { // issue-state mutation is its whole deliverable — hence the shared // non-committing-coordinator posture above. On-demand by default — a manual // Run is the explicit consent to mutate issue state; a cadence is opt-in. + // + // The SAME deterministic pre-step also unblocks `blocked` issues: one whose + // body names its dependency via `Blocked by #N` (portos-file-issue skill) has + // its label removed, with no coordinator dispatch, once every named blocker + // is closed (blockedIssueReconcile.js). Like releaseAbandonedClaims, this + // needs no model and runs every pass regardless of whether any zombie exists. 'issue-reconcile': { type: INTERVAL_TYPES.ON_DEMAND, perpetual: true, enabled: true, providerId: null, model: null, prompt: null, recheckCron: '0 4 * * *', drainDispatchCap: PERPETUAL_DRAIN_DISPATCH_CAP, taskMetadata: { ...NON_COMMITTING_COORDINATOR_METADATA, autoClose: true } }, 'console-errors': { type: INTERVAL_TYPES.ON_DEMAND, enabled: true, providerId: null, model: null, prompt: null, taskMetadata: { fileIssues: false } }, 'dependency-updates': { type: INTERVAL_TYPES.ON_DEMAND, enabled: true, providerId: null, model: null, prompt: null }, @@ -595,7 +601,7 @@ export const TASK_TYPE_DESCRIPTIONS = { 'claim-work': "Ship the next work item from the app's configured tracker (PLAN.md, GitHub/GitLab issues, or JIRA), routed automatically", 'accessibility': 'Accessibility audit — file issues or implement fixes', 'branch-reconcile': "Finish this machine's in-flight local branches: clean up merged ones, open PRs, resolve conflicts, drive review, auto-merge when green", - 'issue-reconcile': "Heal zombie issues: open + in-progress but their PR already merged with no live claim — close + file a scoped follow-up when work remains, or release the claim so the queue re-picks it", + 'issue-reconcile': "Heal zombie issues (open + in-progress but their PR already merged with no live claim — close + file a scoped follow-up or release the claim) and auto-unblock: remove the `blocked` label once every issue named in its `Blocked by #N` line has closed", 'dependency-updates': 'Land or resolve open Dependabot/Renovate PRs, then update the dependencies they missed', 'release-check': 'Check for release readiness', 'error-handling': 'Failure-path audit — file issues or implement fixes',