diff --git a/.github/workflows/dev-release-prep.yml b/.github/workflows/dev-release-prep.yml index c5b100c..ce08411 100644 --- a/.github/workflows/dev-release-prep.yml +++ b/.github/workflows/dev-release-prep.yml @@ -18,7 +18,7 @@ concurrency: jobs: prepare: - if: github.event.pull_request.merged == true && !startsWith(github.event.pull_request.head.ref, 'release/') + if: github.event.pull_request.merged == true && !startsWith(github.event.pull_request.head.ref, 'release/') && !startsWith(github.event.pull_request.head.ref, 'review/') runs-on: ubuntu-latest env: OPENCODE_API_KEY: ${{ secrets.OPENCODE_API_KEY }} diff --git a/.github/workflows/review-response.yml b/.github/workflows/review-response.yml index 6b27802..3b01f2b 100644 --- a/.github/workflows/review-response.yml +++ b/.github/workflows/review-response.yml @@ -13,6 +13,7 @@ jobs: pull-requests: write env: OPENCODE_API_KEY: ${{ secrets.OPENCODE_API_KEY }} + GH_TOKEN: ${{ secrets.PR_AUTOMATION_TOKEN }} steps: - name: Checkout PR head uses: actions/checkout@v4 @@ -20,6 +21,7 @@ jobs: fetch-depth: 0 repository: ${{ github.event.pull_request.head.repo.full_name }} ref: ${{ github.event.pull_request.head.ref }} + token: ${{ env.GH_TOKEN }} - name: Verify OpenCode secret run: | @@ -28,6 +30,13 @@ jobs: exit 1 fi + - name: Validate PR automation token + run: | + if [ -z "$GH_TOKEN" ]; then + echo "PR_AUTOMATION_TOKEN is required to open PRs; org policy blocks GITHUB_TOKEN" >&2 + exit 1 + fi + - name: Setup Node.js uses: actions/setup-node@v4 with: @@ -53,7 +62,7 @@ jobs: - name: Run review-response agent env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ env.GH_TOKEN }} run: | opencode run \ --agent review-response \ @@ -73,7 +82,7 @@ jobs: - name: Commit and push if: steps.diff.outputs.has_changes == 'true' env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ env.GH_TOKEN }} run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" @@ -84,7 +93,7 @@ jobs: - name: Open pull request if: steps.diff.outputs.has_changes == 'true' env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_TOKEN: ${{ env.GH_TOKEN }} run: | comment_url="${{ steps.context.outputs.comment_url }}" reviewer="${{ steps.context.outputs.reviewer }}" diff --git a/lib/prompts/codex.ts b/lib/prompts/codex.ts index df6fae7..2d5ff20 100644 --- a/lib/prompts/codex.ts +++ b/lib/prompts/codex.ts @@ -67,8 +67,13 @@ function readCachedInstructions( cacheFilePath: string, etag?: string | undefined, tag?: string | undefined, -): string { - const fileContent = safeReadFile(cacheFilePath) || ""; +): string | null { + const fileContent = safeReadFile(cacheFilePath); + if (!fileContent) { + logWarn("Cached Codex instructions missing or empty; skipping session cache"); + return null; + } + cacheSessionEntry(fileContent, etag, tag); return fileContent; } @@ -101,7 +106,11 @@ async function fetchInstructionsFromGithub( const response = await fetch(url, { headers }); if (response.status === 304 && cacheFileExists) { - return readCachedInstructions(cacheFilePath, cachedETag || undefined, latestTag); + const cachedContent = readCachedInstructions(cacheFilePath, cachedETag || undefined, latestTag); + if (cachedContent) { + return cachedContent; + } + throw new Error("Cached Codex instructions were unavailable after 304 response"); } if (!response.ok) { @@ -153,11 +162,15 @@ async function fetchInstructionsWithFallback( if (options.cacheFileExists) { logWarn("Using cached instructions due to fetch failure"); - return readCachedInstructions( + const cachedContent = readCachedInstructions( options.cacheFilePath, options.effectiveEtag || options.cachedETag || undefined, options.cachedTag || undefined, ); + if (cachedContent) { + return cachedContent; + } + logWarn("Cached instructions unavailable; falling back to bundled instructions"); } logWarn("Falling back to bundled instructions"); @@ -196,7 +209,15 @@ export async function getCodexInstructions(): Promise { const cacheFileExists = fileExistsAndNotEmpty(cacheFilePath); if (cacheIsFresh(cachedTimestamp, cacheFileExists)) { - return readCachedInstructions(cacheFilePath, cachedETag || undefined, cachedTag || undefined); + const cachedContent = readCachedInstructions( + cacheFilePath, + cachedETag || undefined, + cachedTag || undefined, + ); + if (cachedContent) { + return cachedContent; + } + logWarn("Cached Codex instructions were empty; attempting to refetch"); } let latestTag: string | undefined; @@ -207,7 +228,15 @@ export async function getCodexInstructions(): Promise { error, }); if (cacheFileExists) { - return readCachedInstructions(cacheFilePath, cachedETag || undefined, cachedTag || undefined); + const cachedContent = readCachedInstructions( + cacheFilePath, + cachedETag || undefined, + cachedTag || undefined, + ); + if (cachedContent) { + return cachedContent; + } + logWarn("Cached instructions unavailable; falling back to bundled copy"); } return loadBundledInstructions(); } diff --git a/lib/request/compaction-helpers.ts b/lib/request/compaction-helpers.ts index 2af8e42..0fb181f 100644 --- a/lib/request/compaction-helpers.ts +++ b/lib/request/compaction-helpers.ts @@ -8,7 +8,6 @@ import { import type { CompactionDecision } from "../compaction/compaction-executor.js"; import { filterInput } from "./input-filters.js"; import type { InputItem, RequestBody } from "../types.js"; -import { cloneInputItems } from "../utils/clone.js"; import { countConversationTurns } from "../utils/input-item-utils.js"; export interface CompactionSettings { @@ -24,15 +23,16 @@ export interface CompactionOptions { preserveIds?: boolean; } +/** + * Drop only the latest user message (e.g., a compaction command) while preserving any later assistant/tool items. + */ function removeLastUserMessage(items: InputItem[]): InputItem[] { - const cloned = cloneInputItems(items); - for (let index = cloned.length - 1; index >= 0; index -= 1) { - if (cloned[index]?.role === "user") { - cloned.splice(index, 1); - break; + for (let index = items.length - 1; index >= 0; index -= 1) { + if (items[index]?.role === "user") { + return [...items.slice(0, index), ...items.slice(index + 1)]; } } - return cloned; + return items; } function maybeBuildCompactionPrompt( @@ -43,9 +43,7 @@ function maybeBuildCompactionPrompt( if (!settings.enabled) { return null; } - const conversationSource = commandText - ? removeLastUserMessage(originalInput) - : cloneInputItems(originalInput); + const conversationSource = commandText ? removeLastUserMessage(originalInput) : originalInput; const turnCount = countConversationTurns(conversationSource); let trigger: "command" | "auto" | null = null; let reason: string | undefined; diff --git a/lib/request/input-filters.ts b/lib/request/input-filters.ts index a219da9..f3720d7 100644 --- a/lib/request/input-filters.ts +++ b/lib/request/input-filters.ts @@ -13,6 +13,8 @@ import type { InputItem, SessionContext } from "../types.js"; import { extractTextFromItem } from "../utils/input-item-utils.js"; import { logDebug } from "../logger.js"; +const TOOL_REMAP_MESSAGE_HASH = generateContentHash(TOOL_REMAP_MESSAGE); + export function filterInput( input: InputItem[] | undefined, options: { preserveIds?: boolean } = {}, @@ -247,6 +249,17 @@ export function addToolRemapMessage( ): InputItem[] | undefined { if (!hasTools || !Array.isArray(input)) return input; + const hasExistingToolRemap = input.some((item) => { + if (item?.type !== "message" || item?.role !== "developer") return false; + const contentText = extractTextFromItem(item); + if (!contentText) return false; + return generateContentHash(contentText) === TOOL_REMAP_MESSAGE_HASH; + }); + + if (hasExistingToolRemap) { + return input; + } + const toolRemapMessage: InputItem = { type: "message", role: "developer", diff --git a/package-lock.json b/package-lock.json index a0daf70..2c5dc17 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@openhax/codex", - "version": "0.3.0", + "version": "0.3.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@openhax/codex", - "version": "0.3.0", + "version": "0.3.5", "license": "GPL-3.0-only", "dependencies": { "@openauthjs/openauth": "^0.4.3", diff --git a/scripts/review-response-context.mjs b/scripts/review-response-context.mjs index c1e7212..ddad10e 100644 --- a/scripts/review-response-context.mjs +++ b/scripts/review-response-context.mjs @@ -36,10 +36,10 @@ function main() { const filePath = comment.path; const reviewer = comment.user?.login ?? "unknown"; - const branchSlug = `review/comment-${comment.id}`; + const baseRef = pr.base?.ref ?? "main"; + const branchSlug = `review/${baseRef}-${comment.id}`; const prNumber = pr.number; const prTitle = pr.title ?? ""; - const baseRef = pr.base?.ref ?? "main"; const baseSha = pr.base?.sha ?? ""; const headRef = pr.head?.ref ?? ""; const headSha = pr.head?.sha ?? ""; diff --git a/spec/review-response-token.md b/spec/review-response-token.md new file mode 100644 index 0000000..eb5b823 --- /dev/null +++ b/spec/review-response-token.md @@ -0,0 +1,24 @@ +# Review-response workflow token alignment + +## Context + +- File: .github/workflows/review-response.yml (lines 1-106) +- Behavior: Responds to PR review comments; checks out PR head and pushes auto-fix branch using default GITHUB_TOKEN. +- Issue: Org policy blocks PR creation/push via default `GITHUB_TOKEN`; dev-release-prep uses `PR_AUTOMATION_TOKEN` validated explicitly. +- Existing related workflow: .github/workflows/dev-release-prep.yml uses `PR_AUTOMATION_TOKEN` for PR creation and validates secret. + +## Requirements / Definition of Done + +- review-response workflow uses `secrets.PR_AUTOMATION_TOKEN` for all GitHub write operations (push, PR creation, gh api/cli) instead of default GITHUB_TOKEN. +- Validate presence of PR_AUTOMATION_TOKEN early with a failure message mirroring dev-release-prep wording. +- Keep OPENCODE_API_KEY handling unchanged. +- Ensure checkout/push/pr steps reference the new token via env (GH_TOKEN and git auth) so branch push + PR creation succeed under org policy. +- Review-response branches should be named `review/-`. +- Release automation should treat `review/*` branches as non-release and avoid version bumps. +- Tests/build unaffected (workflow-only change). + +## Open Questions / Notes + +- No other review-response workflows present. +- PAT must have repo permissions; assumes secret already configured in repo/org. +- Checkout of fork PRs may still require permission; pushing uses PAT. diff --git a/spec/review-v0.3.5-fixes.md b/spec/review-v0.3.5-fixes.md new file mode 100644 index 0000000..a2ba497 --- /dev/null +++ b/spec/review-v0.3.5-fixes.md @@ -0,0 +1,25 @@ +# Review v0.3.5 fixes + +## Scope + +- Handle null/empty cache reads in `lib/prompts/codex.ts` around readCachedInstructions caching logic +- Remove redundant cloning in `lib/request/compaction-helpers.ts` (removeLastUserMessage, maybeBuildCompactionPrompt) +- Prevent duplicate tool remap injection in `lib/request/input-filters.ts` addToolRemapMessage + +## Existing issues / PRs + +- None identified for this branch (review/v0.3.5). + +## Definition of done + +- safeReadFile null results do not get cached as empty content; fallback logic remains available for caller +- Compaction helpers avoid unnecessary clones while preserving immutability semantics (original input reused unless truncated) +- Tool remap message is only prepended once when tools are present; logic handles undefined/null safely +- All relevant tests updated or added if behavior changes; existing suite passes locally if run + +## Requirements / notes + +- Only cache instructions when actual non-empty content is read; on null either warn and return null or allow existing fallback paths +- removeLastUserMessage should find last user role index and slice; when commandText is falsy reuse originalInput directly in maybeBuildCompactionPrompt +- addToolRemapMessage should fingerprint or compare TOOL_REMAP_MESSAGE and skip if already present (matching role/type/text) +- Preserve existing function signatures and return types throughout diff --git a/test/compaction-helpers.test.ts b/test/compaction-helpers.test.ts new file mode 100644 index 0000000..980475a --- /dev/null +++ b/test/compaction-helpers.test.ts @@ -0,0 +1,59 @@ +import { applyCompactionIfNeeded } from "../lib/request/compaction-helpers.js"; +import type { InputItem, RequestBody } from "../lib/types.js"; + +describe("compaction helpers", () => { + it("drops only the last user command and keeps trailing items", () => { + const originalInput: InputItem[] = [ + { type: "message", role: "assistant", content: "previous response" }, + { type: "message", role: "user", content: "/codex-compact please" }, + { type: "message", role: "assistant", content: "trailing assistant" }, + ]; + const body: RequestBody = { model: "gpt-5", input: [...originalInput] }; + + const decision = applyCompactionIfNeeded(body, { + settings: { enabled: true }, + commandText: "codex-compact please", + originalInput, + }); + + expect(decision?.mode).toBe("command"); + expect(decision?.serialization.transcript).toContain("previous response"); + expect(decision?.serialization.transcript).toContain("trailing assistant"); + expect(decision?.serialization.transcript).not.toContain("codex-compact please"); + + // Verify RequestBody mutations + expect(body.input).not.toEqual(originalInput); + expect(body.input?.some((item) => item.content === "/codex-compact please")).toBe(false); + expect((body as any).tools).toBeUndefined(); + expect((body as any).tool_choice).toBeUndefined(); + expect((body as any).parallel_tool_calls).toBeUndefined(); + }); + + it("applies compaction when no user message exists", () => { + const originalInput: InputItem[] = [ + { + type: "message", + role: "assistant", + content: "system-only follow-up", + }, + ]; + const body: RequestBody = { model: "gpt-5", input: [...originalInput] }; + + const decision = applyCompactionIfNeeded(body, { + settings: { enabled: true }, + commandText: "codex-compact", + originalInput, + }); + + expect(decision?.serialization.totalTurns).toBe(1); + expect(decision?.serialization.transcript).toContain("system-only follow-up"); + + // Verify RequestBody mutations + expect(body.input).toBeDefined(); + expect(body.input?.length).toBeGreaterThan(0); + expect(body.input).not.toEqual(originalInput); + expect((body as any).tools).toBeUndefined(); + expect((body as any).tool_choice).toBeUndefined(); + expect((body as any).parallel_tool_calls).toBeUndefined(); + }); +});