Skip to content

chore: init core docs including architecture decision records and initial specs#1

Merged
BjornMelin merged 78 commits into
mainfrom
chore/init-core-docs
Jan 25, 2026
Merged

chore: init core docs including architecture decision records and initial specs#1
BjornMelin merged 78 commits into
mainfrom
chore/init-core-docs

Conversation

@BjornMelin

Copy link
Copy Markdown
Owner

This pull request sets up foundational project scaffolding and documentation for the Codex ToolLoop repository, establishing standards for development, configuration, and usage. It introduces key documentation (including a PRD and ADR), enforces Node.js v24 and pnpm as requirements, and clarifies configuration for MCP integration. The most important changes are grouped below:

Project Documentation and Guidelines

  • Added a comprehensive Product Requirements Document (docs/PRD.md) outlining product goals, user personas, use cases, functional and non-functional requirements, and a phased delivery plan.
  • Replaced AGENTS.md with detailed repository guidelines covering setup, project structure, development standards, coding style, and commit conventions.
  • Added an Architecture Decision Record (docs/adr/0001-runtime-node-pnpm.md) specifying Node.js v24 LTS and pnpm as required for development and runtime, with rationale and consequences.

Configuration and Environment

  • Updated .env.example to include configuration variables for MCP (tool substrate) integration, supporting both file path and inline JSON configuration.
  • Added .nvmrc to pin the Node.js version to 24, ensuring consistent runtime across environments.

Formatting and Licensing

  • Changed biome.json to enforce an 80-character line width for code formatting, aligning with documented style guidelines.
  • Added a standard MIT License file to the repository.
  • Minor whitespace cleanup in .editorconfig.

The Codex SDK and non-interactive mode documentation have been removed as part of the cleanup process. These changes streamline the documentation by eliminating outdated or redundant content.
…, dynamic tool loading, Codex backends, agent runtime, CLI, and optional Next.js UI
- Changed package manager from bun to pnpm.
- Added engines field to specify Node.js version requirement.
- Configured workspaces for apps and packages.
- Updated scripts to use pnpm and tsx for running examples.
- Adjusted TypeScript configuration for module resolution and included additional directories.
- Enhanced Vitest configuration to include type checking for test definitions.
@BjornMelin BjornMelin self-assigned this Jan 22, 2026
@BjornMelin BjornMelin added the documentation Improvements or additions to documentation label Jan 22, 2026
Copilot AI review requested due to automatic review settings January 22, 2026 03:19
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jan 22, 2026

Copy link
Copy Markdown

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The PR renames the project to codex-toolloop and migrates tooling from Bun to pnpm (adds .nvmrc with Node 24.11). The root package.json is updated (name, version, license, engines, workspaces, scripts, deps/devDeps). Monorepo tooling and config were added or adjusted (Turborepo, Biome, ESLint, Vitest, eslint.config.js). New packages/apps include apps/cli (doctor command) and packages/codex-app-server-protocol. Extensive documentation, ADRs, and SPECs were added or reorganized; README and LICENSE were created. CI and security automation were added (GitHub workflows, Dependabot, CodeQL, Scorecard). Misc: .env.example, .gitignore, examples, and minor formatting edits.

🚥 Pre-merge checks | ✅ 5 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 59.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Docstring & Research Standards ⚠️ Warning Pull request fails two mandatory quality gates: Gate 4.2.1 requires doc comment for exported declaration in eslint.config.js; Gate 5 prohibits Unicode EM DASH in docs/adr/0001-runtime-node-pnpm.md. Add doc comment to export default defineConfig([...]) in eslint.config.js; replace EM DASH with -- on line 26 of docs/adr/0001-runtime-node-pnpm.md.
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'chore: init core docs including architecture decision records and initial specs' accurately describes the primary change of adding foundational documentation, ADRs, and specifications.
Description check ✅ Passed The pull request description is directly related to the changeset, detailing the added documentation, configuration changes, and enforcement of development standards.
React + Next.Js Best Practices ✅ Passed This PR contains no React or Next.js implementation code, only future-marked specifications and foundational documentation.
Web Interface Guidelines ✅ Passed The Web Interface Guidelines check is not applicable to this pull request. No .tsx, .jsx, .html, or CSS/SCSS/LESS files are being added or modified. The PR consists entirely of documentation, configuration files, CLI code, and repository scaffolding.
Google Python Style Compliance ✅ Passed The Google Python Style Guide compliance check is not applicable. The repository contains no Python files (.py extension), only TypeScript, Markdown, JSON, YAML, and JavaScript files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/init-core-docs

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e73ea81 and 54dc715.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml
🧰 Additional context used
🔍 Remote MCP Firecrawl

Summary of external facts relevant to this PR (sources cited):

  • Node v24.11.0 ("Krypton") is Active LTS and will receive updates through April 2028 — matching the PR's decision to pin Node 24 in .nvmrc / ADR.

  • pnpm v10 provides a security-by-default model and an allowBuilds configuration (introduced in v10.26) that is the preferred replacement for onlyBuiltDependencies/ignoredBuiltDependencies. allowBuilds is a map of package matchers to allow/disallow lifecycle script execution; packages not listed are disallowed by default (strictDepBuilds can turn that into an error). The pnpm settings docs also document related supply-chain controls (minimumReleaseAge, trustPolicy, blockExoticSubdeps) that the PR's ADRs reference. (Also: an earlier CLI bug around writing allowBuilds was fixed in pnpm; confirm workspace usage matches v10 behavior.)

  • Zod v4 (zod@4.3.6 referenced in the PR) introduces breaking changes and API migrations: string-format validators (email, uuid, url, etc.) are promoted to top-level z.* APIs (z.email(), z.uuid(), …) and the older method forms (z.string().email()) are deprecated; error customization moved to a unified error param; several other semantic changes exist (see migration guide). Code using Zod should be reviewed for v4 migration issues (top-level validators, error param changes, changed defaults).

🔇 Additional comments (5)
.github/workflows/ci.yml (5)

1-15: Top-level workflow controls look solid. Explicit permissions and concurrency are well-scoped.


17-34: Lint job setup is clean and deterministic. Composite setup plus PR-scoped Turbo filter is appropriate.


35-52: Typecheck job mirrors lint correctly. Consistent setup and filter usage.


53-70: Test job pattern is consistent. Clear PR scoping and shared setup.


71-90: Build gating on lint/typecheck is a good safeguard. The build + gen check invocation is clear.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request establishes foundational scaffolding and documentation for the Codex ToolLoop repository, transitioning from Bun to pnpm and Node.js v24 LTS. It introduces comprehensive project documentation including a Product Requirements Document, Architecture Decision Records, and detailed implementation specifications.

Changes:

  • Migrated build system from Bun to pnpm with Node.js v24 LTS requirement
  • Added comprehensive documentation structure (PRD, ADRs, specs) covering architecture, MCP integration, multi-agent workflows, and implementation roadmap
  • Updated TypeScript configuration to use NodeNext module resolution and added workspace support for monorepo structure

Reviewed changes

Copilot reviewed 40 out of 63 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
package.json Updated project name, added version, changed package manager to pnpm, updated scripts and dependencies
tsconfig.json Changed to NodeNext module resolution, added exactOptionalPropertyTypes, expanded include paths for monorepo
vitest.config.ts Added unit and type test paths, enabled typecheck for tests
pnpm-workspace.yaml New workspace configuration with apps/* and packages/*, allowBuilds for biome and esbuild
pnpm-lock.yaml New lockfile with all dependencies for Node.js v24 environment
biome.json Changed line width from 100 to 80 characters
src/lib/create-codex-from-env.ts Added JSDoc documentation for types and function
docs/specs/*.md Added 7 detailed specification documents covering bootstrap, MCP, dynamic loading, backends, runtime, CLI, and memory
docs/adr/index.md Added ADR index referencing 9 architecture decisions
docs/mcp.md Updated to describe MCP usage in Codex ToolLoop context
docs/getting-started.md Updated installation commands from bun to pnpm
docs/reference/*.md Removed 5 reference documentation files (non-interactive, SDK docs, CLI options)
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread package.json Outdated
Comment thread package.json Outdated
coderabbitai[bot]

This comment was marked as outdated.

… configuration; update security and maintenance documentation
coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as outdated.

…ows`, `cli`, and `testkit` packages, dynamic tool loading, and event system.
coderabbitai[bot]

This comment was marked as outdated.

…odex

feat(examples): update examples to improve clarity and structure
chore: update pnpm lockfile for dependency versioning
coderabbitai[bot]

This comment was marked as resolved.

chore(docs): enhance ADRs with media handling and transport implications
coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@BjornMelin

Copy link
Copy Markdown
Owner Author

Resolved threads: 25 | Files: .gitignore, biome.json, docs/adr/0002-model-backends.md, docs/specs/010-mcp-platform.md, docs/specs/024-codex-app-server-endpoints.md, docs/specs/026-codex-approvals-permissions.md, docs/specs/027-codex-skills.md, docs/specs/028-codex-config-layering.md, docs/specs/029-ai-sdk-ui-stream-contract.md, docs/specs/060-optional-next-ui.md, docs/specs/080-turborepo-monorepo.md, packages/codex-app-server-protocol/package.json, packages/codex-app-server-protocol/src/index.ts, packages/codex/src/app-server/ui-stream.ts, packages/codex/src/exec-events.ts, tests/unit/codex-app-server-event-mapper.test.ts | Tests: pnpm -s fix:typecheck | Notes: refreshed protocol exports and doc clarity per review.

coderabbitai[bot]

This comment was marked as resolved.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@docs/adr/0002-model-backends.md`:
- Around line 32-37: Update the backend name to reflect the 2026-01-23
amendment: replace occurrences of "AI SDK Codex App Server provider backend"
with a clearer name like "Codex App Server (JSONL-over-stdio) backend" or
similar, and remove the "AI SDK" prefix in any headings/descriptions (see the
phrase "AI SDK Codex App Server provider backend" and the amendment note
describing "JSONL-over-stdio client for `codex app-server`") so the doc
accurately indicates the current implementation and avoids implying reliance on
the AI SDK community provider.

In `@docs/specs/080-turborepo-monorepo.md`:
- Around line 143-146: Remove the "(UNVERIFIED)" marker from the guidance line
that currently reads "Recommended: use a dedicated token for CI and rotate it
periodically (UNVERIFIED)." and make it definitive by updating it to
"Recommended: use a dedicated token for CI and rotate it periodically." so the
doc states Vercel's best-practice guidance clearly.
♻️ Duplicate comments (1)
docs/specs/029-ai-sdk-ui-stream-contract.md (1)

32-39: Resolve tool-approval chunk contradiction in the contract.

The spec says approvals emit tool-approval-request chunks (Lines 47-49) but also says approvals do not emit a dedicated chunk type (Lines 57-58), and the chunk list includes tool-approval-request (Lines 32-39). This is internally inconsistent and will produce conflicting implementations. Align the chunk list and required contract with the “no dedicated chunk type” note by documenting approval as part state only.

🛠️ Suggested fix (align with part-state approval)
-- `ui-message-chunks.ts` -- stream chunk types (`start`, `text-start`,
-  `text-delta`, `text-end`, `reasoning-start`, `reasoning-delta`,
-  `reasoning-end`, `error`, `tool-input-start`, `tool-input-delta`,
-  `tool-input-available`, `tool-input-error`, `tool-approval-request`,
-  `tool-output-available`, `tool-output-error`, `tool-output-denied`,
-  `source-url`, `source-document`, `file`, `data-*`, `start-step`,
-  `finish-step`, `finish`, `abort`, `message-metadata`).
+- `ui-message-chunks.ts` -- stream chunk types (`start`, `text-start`,
+  `text-delta`, `text-end`, `reasoning-start`, `reasoning-delta`,
+  `reasoning-end`, `error`, `tool-input-start`, `tool-input-delta`,
+  `tool-input-available`, `tool-input-error`, `tool-output-available`,
+  `tool-output-error`, `tool-output-denied`, `source-url`,
+  `source-document`, `file`, `data-*`, `start-step`, `finish-step`,
+  `finish`, `abort`, `message-metadata`).
...
-- Tool approval requests for AI SDK tools emit `tool-approval-request` chunks,
-  which transition tool parts to `approval-requested`; Codex-native approvals
-  emit `data-codex-event` parts.
+- Tool approval requests for AI SDK tools transition tool parts to
+  `approval-requested` (no dedicated chunk type). Codex-native approvals emit
+  `data-codex-event` parts.

Also applies to: 47-49, 57-58

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 916c573 and 65233e4.

📒 Files selected for processing (6)
  • docs/adr/0002-model-backends.md
  • docs/specs/026-codex-approvals-permissions.md
  • docs/specs/027-codex-skills.md
  • docs/specs/029-ai-sdk-ui-stream-contract.md
  • docs/specs/060-optional-next-ui.md
  • docs/specs/080-turborepo-monorepo.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-22T20:51:16.701Z
Learnt from: BjornMelin
Repo: BjornMelin/codex-toolloop PR: 1
File: docs/specs/060-optional-next-ui.md:107-108
Timestamp: 2026-01-22T20:51:16.701Z
Learning: In the codex-toolloop repository, enforce using double hyphens (--) instead of em dashes (—) in all Markdown documentation. Em dashes are considered an AI antipattern and should be avoided. Apply this guideline across all docs under the docs directory (not just the specified file) and replace any em dashes with -- for consistency and readability.

Applied to files:

  • docs/specs/060-optional-next-ui.md
  • docs/specs/080-turborepo-monorepo.md
  • docs/adr/0002-model-backends.md
  • docs/specs/027-codex-skills.md
  • docs/specs/026-codex-approvals-permissions.md
  • docs/specs/029-ai-sdk-ui-stream-contract.md
🪛 LanguageTool
docs/specs/060-optional-next-ui.md

[style] ~34-~34: Consider using the typographical ellipsis character here instead.
Context: ...e CLI locally. In development it runs pnpm dev:cli -- run workflow ...; in production it should call a buil...

(ELLIPSIS)


[style] ~35-~35: Consider using the typographical ellipsis character here instead.
Context: ...t/installed CLI entrypoint (for example codex-toolloop run workflow ...) or be disabled if the UI is distrib...

(ELLIPSIS)


[style] ~41-~41: Consider using the typographical ellipsis character here instead.
Context: ...installed CLI entrypoint (for example codex-toolloop run workflow ...). If the UI is distributed without a...

(ELLIPSIS)


[typographical] ~125-~125: Consider using an em dash (—) instead of ‘--’.
Context: .... Required fields: - runId: string` -- run identifier, matches run directory n...

(TWO_HYPHENS)


[typographical] ~126-~126: Consider using an em dash (—) instead of ‘--’.
Context: ...hes run directory name. - pid: number -- spawned process ID. - `startTime: strin...

(TWO_HYPHENS)


[typographical] ~127-~127: Consider using an em dash (—) instead of ‘--’.
Context: ...awned process ID. - startTime: string -- ISO timestamp. - `status: "running" | "...

(TWO_HYPHENS)


[style] ~162-~162: ‘new records’ might be wordy. Consider a shorter alternative.
Context: ... - Include schemaVersion?: number in new records when fields change; maintain backward...

(EN_WORDINESS_PREMIUM_NEW_RECORDS)

docs/specs/080-turborepo-monorepo.md

[uncategorized] ~149-~149: The official name of this software platform is spelled with a capital “H”.
Context: ...pose both variables to the Turbo job in .github/workflows/ci.yml: ```yaml env: TURB...

(GITHUB)


[style] ~179-~179: Consider using the typographical ellipsis character here instead.
Context: ... single CI job. - On pull requests, use --filter=...[origin/<base-branch>] to skip unaffect...

(ELLIPSIS)


[typographical] ~196-~196: In American English, use a period after an abbreviation.
Context: ...tools/biome) - [Turborepo config (tasks vs pipeline)](https://turborepo.dev/docs/r...

(MISSING_PERIOD_AFTER_ABBREVIATION)

docs/adr/0002-model-backends.md

[typographical] ~95-~95: Consider using an em dash (—) instead of ‘--’.
Context: ...ection). ## Amendments ### 2026-01-21 -- Codex backend v1 implemented This ADR ...

(TWO_HYPHENS)


[typographical] ~113-~113: Consider using an em dash (—) instead of ‘--’.
Context: ...penai.com/codex/config> ### 2026-01-23 -- App-server backend migrated to internal...

(TWO_HYPHENS)


[typographical] ~125-~125: Consider using an em dash (—) instead of ‘--’.
Context: ...types.md- SPEC 023: Protocol workflow --docs/specs/023-codex-app-server-protoc...

(TWO_HYPHENS)

docs/specs/029-ai-sdk-ui-stream-contract.md

[typographical] ~32-~32: Consider using an em dash (—) instead of ‘--’.
Context: ... defined by AI SDK: - ui-messages.ts -- UIMessage parts and tool approval state...

(TWO_HYPHENS)


[typographical] ~33-~33: Consider using an em dash (—) instead of ‘--’.
Context: ...proval states. - ui-message-chunks.ts -- stream chunk types (start, `text-star...

(TWO_HYPHENS)


[typographical] ~40-~40: Consider using an em dash (—) instead of ‘--’.
Context: ...ata). - process-ui-message-stream.ts` -- state machine for approvals and tool ca...

(TWO_HYPHENS)

🔍 Remote MCP Firecrawl

Summary of additional facts relevant to reviewing this PR

  • pnpm allowBuilds / build-script model: pnpm v10 introduced allowBuilds (single map replacing onlyBuiltDependencies/ignoredBuiltDependencies). Packages not listed are disallowed by default; strictDepBuilds makes unreviewed build-scripts an error. There is an approve-builds helper to pick allowed packages. See pnpm settings and supply-chain docs.,

  • Node.js 24.11.0 (LTS) notes: Node 24.11.0 is LTS ("Krypton"). Node 24 uses OpenSSL 3.5 with default security level 2 (disallows weak keys/ciphers); additional behavioral/argument-validation changes and build toolchain requirements (GCC/Xcode). Migration/codmod guidance available. [.nvmrc pinned to 24.11 in PR aligns with this.],

  • @openai/codex-sdk v0.89.0 (used/added in package.json): the TypeScript SDK wraps/spawns the codex CLI (exchanges JSONL over stdin/stdout), requires Node.js 18+, supports streaming (runStreamed), structured output via JSON Schema, resuming threads, and explicit env control when creating the client. This matches PR changes that reference MCP/SDK and structured outputs.,

Tools used: Firecrawl (firecrawl_search) for pnpm, Node.js, and Codex SDK docs.

🔇 Additional comments (5)
docs/specs/080-turborepo-monorepo.md (1)

59-62: The snippet is already correct and matches the actual turbo.json.

The lint, typecheck, and test tasks intentionally omit "cache": false in both the root turbo.json (lines 21–28) and the packages/repo-tasks extension, meaning they use Turborepo's default caching behavior. The documentation accurately reflects this configuration, so no change is needed.

Separate concern: The (UNVERIFIED) marker at line 145 regarding token rotation should either be sourced or removed. The Vercel/Turborepo documentation should be consulted to confirm whether rotating CI tokens is a recommended practice.

docs/adr/0002-model-backends.md (4)

1-27: Clear context and well-structured ADR.

The status, context, needs, and constraints are clearly articulated. The integration options are well-described and provide good background for the decision.


54-76: Solid alternatives analysis and realistic consequences.

The alternatives are well-reasoned with balanced pros/cons, and the consequences appropriately acknowledge both the complexity cost and the flexibility benefits.


78-91: Clear implementation guidance.

The normalized event model approach and backend capability matrix are well-articulated. The references correctly reflect the current state (no injection support).


93-125: All referenced documents and API parameters are correct. ADR 0013 (docs/adr/0013-codex-app-server-protocol-types.md) and SPEC 023 (docs/specs/023-codex-app-server-protocol.md) exist and are properly cited. The CLI flags (--output-schema, --output-last-message) and reasoningEffort values are accurately described and match current Codex API documentation.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread docs/adr/0002-model-backends.md Outdated
Comment thread docs/specs/080-turborepo-monorepo.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@docs/adr/0002-model-backends.md`:
- Around line 15-21: The three consecutive bullet points in the Codex
integration list start with the same word and feel repetitive; rephrase each
bullet (the lines describing "AI SDK Codex CLI provider", "Codex App Server
(JSONL-over-stdio)", "Codex exec", and "Codex TypeScript SDK") to vary sentence
openings—e.g., start some bullets with the capability or behavior ("Supports
persistent threads", "Stable CLI automation") or the integration type ("AI SDK
provider", "TypeScript SDK")—while preserving the original meaning and
references to JSONL, stdio, mid-execution injection, output schema, and
resumable threads.
- Around line 109-111: The markdown list contains a broken link
"https://developers.openai.com/codex/config" (Codex config reference); remove
that list item or replace it with an updated working URL if you have one, and
ensure the remaining links
("https://ai-sdk.dev/providers/community-providers/codex-app-server#codex-cli-app-server-provider"
and "https://developers.openai.com/codex/cli/reference/") remain unchanged;
update the surrounding text accordingly so the documentation no longer points to
the 404 resource.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65233e4 and 7c448e6.

📒 Files selected for processing (1)
  • docs/adr/0002-model-backends.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-22T20:51:16.701Z
Learnt from: BjornMelin
Repo: BjornMelin/codex-toolloop PR: 1
File: docs/specs/060-optional-next-ui.md:107-108
Timestamp: 2026-01-22T20:51:16.701Z
Learning: In the codex-toolloop repository, enforce using double hyphens (--) instead of em dashes (—) in all Markdown documentation. Em dashes are considered an AI antipattern and should be avoided. Apply this guideline across all docs under the docs directory (not just the specified file) and replace any em dashes with -- for consistency and readability.

Applied to files:

  • docs/adr/0002-model-backends.md
🪛 LanguageTool
docs/adr/0002-model-backends.md

[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...omation with JSONL and output schema. - Codex TypeScript SDK: programmatic control an...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)


[typographical] ~95-~95: Consider using an em dash (—) instead of ‘--’.
Context: ...ection). ## Amendments ### 2026-01-21 -- Codex backend v1 implemented This ADR ...

(TWO_HYPHENS)


[typographical] ~113-~113: Consider using an em dash (—) instead of ‘--’.
Context: ...penai.com/codex/config> ### 2026-01-23 -- App-server backend migrated to internal...

(TWO_HYPHENS)


[typographical] ~125-~125: Consider using an em dash (—) instead of ‘--’.
Context: ...types.md- SPEC 023: Protocol workflow --docs/specs/023-codex-app-server-protoc...

(TWO_HYPHENS)

🔍 Remote MCP

Here are the relevant concrete facts found to assist in reviewing the pull request:

  1. pnpm allowBuilds and supply chain security:
  • pnpm v10 introduced the allowBuilds configuration which replaced the older onlyBuiltDependencies and ignoredBuiltDependencies.
  • allowBuilds is a map where package matchers are explicitly allowed (true) or disallowed (false) for running lifecycle scripts during install.
  • Packages not listed in allowBuilds are disallowed by default, with warnings or errors based on the strictDepBuilds setting.
  • This is a security feature to block potentially malicious postinstall scripts in unreviewed dependencies.
  • See https://pnpm.io/next/cli/approve-builds and https://pnpm.io/supply-chain-security
  1. Node.js 24.11.0 "Krypton" LTS release:
  • Node.js 24.11.0 is the LTS release "Krypton" with maintenance until April 2028.
  • Uses OpenSSL 3.5 with a default security level of 2 enforcing higher cryptographic standards:
    • RSA/DSA/DH keys shorter than 2048 bits, ECC keys shorter than 224 bits are prohibited.
    • RC4 cipher suites are no longer permitted.
  • API validation is stricter, catching errors earlier.
  • ESM and CommonJS interoperability improved.
  • Updated compiler requirements (GCC 12.2+, Xcode 16.1+).
  • Release notes and changelogs: https://nodejs.org/en/blog/release/v24.11.0 and https://nodejs.org/en/blog/migrations/v22-to-v24
  1. @openai/codex-sdk v0.89.0 (the SDK added/used in the PR):
  • It is a TypeScript SDK for Codex APIs, published 2 days ago.
  • Wraps the bundled codex CLI binary; communicates using JSONL over stdin/stdout.
  • Supports structured JSON schema output by specifying an outputSchema JSON object per turn.
  • Supports streaming responses via runStreamed().
  • Allows thread persistence and resuming, thread pool and caching based on working directory, model, output schema, sandbox mode, and other config.
  • Enables sandbox modes (read-only, workspace-write, danger-full-access).
  • Requires a Git repository by default but can skip the check (unsafe).
  • Supports control over environment variables and Codex CLI binary path.
  • Documentation: https://www.npmjs.com/package/@openai/codex-sdk and https://www.promptfoo.dev/docs/providers/openai-codex-sdk/
  1. Codex SDK usage for code review automation (example from OpenAI docs):
  • Shows usage pattern to run code review in CI/CD (e.g., GitHub Actions or Jenkins).
  • Uses a structured output JSON schema for findings with fields for title, body, confidence_score, priority, and code location with exact file path and line ranges.
  • Prompts the Codex agent for actionable review comments.
  • Publishes inline comments on PR files at the given line ranges along with an overall correctness verdict.
  • Ensures exact file and line citations to avoid rejection.
  • Architecture of code review prompt and output schema aligns with patterns seen in this PR summary.
  • See https://developers.openai.com/cookbook/examples/codex/build_code_review_with_codex_sdk/

Summary for PR review relevance:

  • The PR introduces pnpm workspace config updates and depends on Node.js 24.11 (LTS) runtime pinned with .nvmrc.
  • It upgrades package manager to pnpm v10+ that uses the allowBuilds map to control lifecycle script execution for better supply chain security.
  • The Codex SDK v0.89.0 used in the PR complies with structured output schema for code review automation, enabling sophisticated turn-based AI-driven reviews with correctness verdicts.
  • Node.js 24.11 LTS runtime includes security and behavioral improvements relevant for code execution environments in toolchains and CICD.
  • The CI/CD / GitHub Actions / Jenkins Codex code review automation examples correspond with the new features and structured output used in the PR code and docs.

These details provide important context on runtime, package management, security, AI SDK version, and usage patterns that are relevant for the code and documentation added in this pull request.

Sources:

🔇 Additional comments (1)
docs/adr/0002-model-backends.md (1)

97-97: All internal cross-references are valid and present in the repository.

The ADR correctly references SPEC 020 (packages/codex), ADR 0013 (docs/adr/0013-codex-app-server-protocol-types.md), and SPEC 023 (docs/specs/023-codex-app-server-protocol.md). All referenced documents exist in the repository.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread docs/adr/0002-model-backends.md Outdated
Comment thread docs/adr/0002-model-backends.md Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 17-18: Pin the pnpm setup action to the exact pnpm version from
package.json#packageManager (pnpm@10.28.1) instead of leaving
pnpm/action-setup@v3 unpinned; update each job that uses pnpm/action-setup@v3
(there are four occurrences) to include the version input (e.g., use:
pnpm/action-setup@v3 with: version: '10.28.1') so CI reproducibility matches
package.json, leaving actions/setup-node@v4 unchanged.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c448e6 and 4448f93.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml
🧰 Additional context used
🔍 Remote MCP Firecrawl

Relevant facts to assist reviewing this PR

  • pnpm allowBuilds / supply-chain security: pnpm v10+ uses allowBuilds (map of package matchers → true/false) as the single source to permit/disallow dependency lifecycle scripts; packages not listed are disallowed by default (strictDepBuilds can make this an error). (pnpm docs: approve-builds / settings),

  • Node.js runtime pinned by .nvmrc: Node.js 24.11.0 ("Krypton") is an LTS release; notable changes include OpenSSL 3.5 with stricter crypto defaults (security level 2), stricter argument/validation behavior and ESM/CJS interoperability considerations—verify native addon/tooling build requirements and behavior changes when targeting Node 24.11.0 in CI/dev.,

  • @openai/codex-sdk v0.89.0 (dependency added/updated in root package.json):

    • Package wraps the bundled codex CLI and communicates via JSONL over stdin/stdout.
    • Supports structured output via per-turn JSON Schema (outputSchema) and streaming with runStreamed() (async event generator).
    • Requires Node (docs show Node 18+), provides thread start/resume semantics and working-directory controls; relevant to examples, CLI packaging, and the new packages/apps added in this PR.,
  • Codex project release context (0.89.0): recent Codex CLI/SDK releases (0.88/0.89) added features referenced by this PR (skills, permissions, layered config, thread/read, archived thread filtering); the repo’s docs/specs referencing MCP, dynamic tool loading, and SDK usage align with those upstream changes—good to cross-check protocol/schema compatibility and any generated protocol artifacts included under packages/codex-app-server-protocol.,

Notes for reviewers (actionable checks)

  • Ensure repository pnpm/workspace config and CI accommodate pnpm v10 allowBuilds (if CI runs installs, confirm allowBuilds / onlyBuiltDependencies settings or documented approval steps are present).
  • Validate Node 24.11 pinned files (.nvmrc, engines.node) against CI images/builders and native build toolchains (GCC/Xcode requirements) to avoid CI/build breakage.
  • For codex-sdk v0.89.0 usage: verify any calls that expect structured output (outputSchema) or runStreamed semantics match SDK examples and that generated protocol types/schemas (packages/codex-app-server-protocol) are consistent with the SDK/CLI versions referenced.,

Tools/sources used

  • Firecrawl firecrawl_search (pnpm docs, Node.js release/migration docs, @openai/codex-sdk npm page, Codex changelog/releases)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)

1-8: CI triggers are scoped well to main.

Clean and minimal trigger configuration.


23-32: Turbo PR filtering is consistent across jobs.

The PR-only filtering pattern is clear and keeps CI efficient.

Also applies to: 46-55, 69-78, 92-101

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread .github/workflows/ci.yml Outdated
- Remove "(UNVERIFIED)" marker from CI token rotation guidance in turborepo spec
- Vary sentence structure of Codex integration options bullets (0002-model-backends.md)
- Remove broken Codex config reference link (HTTP 404)

Fixes PR #1 review threads.
Pin the pnpm/action-setup@v3 action to the exact version from package.json#packageManager
to ensure CI reproducibility and prevent drift from unpredictable version updates.

Fixes PR #1 review thread about pnpm version pinning.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 10-24: The CI job repeats the same checkout/pnpm/setup-node/pnpm
install steps (seen in the lint job steps block); extract these repeated steps
into a shared reusable workflow or composite action and replace the duplicate
blocks with a single uses: call (e.g., create a reusable workflow like ci-setup
or composite action named github/actions/ci-setup) and update each job (lint and
the other jobs referenced) to call that shared step; ensure the shared action
accepts inputs for node version/fetch-depth/cache so each job can pass overrides
and confirm the pnpm install --frozen-lockfile step stays in the shared action.

In `@docs/adr/0002-model-backends.md`:
- Line 11: Update the ADR text that currently reads "Codex as the primary coding
model using ChatGPT subscription auth." to explicitly state which Codex CLI
authentication methods are supported: either note that both "ChatGPT
subscription (Sign in with ChatGPT OAuth)" and "standard OpenAI API keys" are
supported, or clearly declare ChatGPT subscription as a hard requirement; also
reconcile this change with the existing mention of "existing Codex CLI
authentication" so the document consistently describes supported auth flows
(search for the exact phrase "Codex as the primary coding model using ChatGPT
subscription auth." and "existing Codex CLI authentication" to edit both spots).
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4448f93 and 7fda0b0.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • .github/workflows/codeql.yml
  • docs/adr/0002-model-backends.md
  • docs/specs/080-turborepo-monorepo.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-22T20:51:16.701Z
Learnt from: BjornMelin
Repo: BjornMelin/codex-toolloop PR: 1
File: docs/specs/060-optional-next-ui.md:107-108
Timestamp: 2026-01-22T20:51:16.701Z
Learning: In the codex-toolloop repository, enforce using double hyphens (--) instead of em dashes (—) in all Markdown documentation. Em dashes are considered an AI antipattern and should be avoided. Apply this guideline across all docs under the docs directory (not just the specified file) and replace any em dashes with -- for consistency and readability.

Applied to files:

  • docs/specs/080-turborepo-monorepo.md
  • docs/adr/0002-model-backends.md
🪛 LanguageTool
docs/specs/080-turborepo-monorepo.md

[uncategorized] ~149-~149: The official name of this software platform is spelled with a capital “H”.
Context: ...pose both variables to the Turbo job in .github/workflows/ci.yml: ```yaml env: TURB...

(GITHUB)


[style] ~179-~179: Consider using the typographical ellipsis character here instead.
Context: ... single CI job. - On pull requests, use --filter=...[origin/<base-branch>] to skip unaffect...

(ELLIPSIS)


[typographical] ~196-~196: In American English, use a period after an abbreviation.
Context: ...tools/biome) - [Turborepo config (tasks vs pipeline)](https://turborepo.dev/docs/r...

(MISSING_PERIOD_AFTER_ABBREVIATION)

docs/adr/0002-model-backends.md

[typographical] ~95-~95: Consider using an em dash (—) instead of ‘--’.
Context: ...ection). ## Amendments ### 2026-01-21 -- Codex backend v1 implemented This ADR ...

(TWO_HYPHENS)


[typographical] ~112-~112: Consider using an em dash (—) instead of ‘--’.
Context: ...m/codex/cli/reference/> ### 2026-01-23 -- App-server backend migrated to internal...

(TWO_HYPHENS)


[typographical] ~124-~124: Consider using an em dash (—) instead of ‘--’.
Context: ...types.md- SPEC 023: Protocol workflow --docs/specs/023-codex-app-server-protoc...

(TWO_HYPHENS)

🔇 Additional comments (6)
docs/specs/080-turborepo-monorepo.md (2)

1-199: Static analysis hints can be safely ignored.

The LanguageTool suggestions are false positives:

  • .github is the correct lowercase directory name for GitHub workflows
  • The ... in --filter=...[origin/<base-branch>] is a literal command-line argument syntax, not typographical ellipsis
  • The "vs" abbreviation without a period is acceptable in link titles

59-61: The baseline configuration is accurate and matches the actual turbo.json implementation.

The spec correctly reflects the current turbo.json where lint, typecheck, and test tasks have only "outputs": [] without "cache": false. The configuration is consistent with the implementation, so no update to the spec is needed.

docs/adr/0002-model-backends.md (3)

109-110: All external documentation URLs are currently accessible.

The referenced external links (AI SDK codex app-server provider, Codex CLI reference, and Codex app-server documentation) all return HTTP 200 status and are valid. No action required.


123-124: Internal documentation references are valid.

Both ADR 0013 and SPEC 023 files exist at the specified paths and can be referenced.


105-105: Remove the claim about reasoningEffort type constraints — the app-server provider actually supports all values including xhigh.

The ReasoningEffort type in packages/codex-app-server-protocol/src/generated/ReasoningEffort.ts and CodexReasoningEffort in packages/codex/src/backend.ts both define the full set as "none" | "minimal" | "low" | "medium" | "high" | "xhigh". The app-server backend passes these values through without restriction, so xhigh does not require a workaround.

Likely an incorrect or invalid review comment.

.github/workflows/codeql.yml (1)

29-38: Skip CodeQL Autobuild for JavaScript/TypeScript and use build-mode: none.

Autobuild is only for compiled languages (C/C++, C#, Go, Java/Kotlin, Swift). For JS/TS, use build-mode: none as CodeQL analyzes interpreted code directly. Still run pnpm install --frozen-lockfile beforehand so CodeQL can resolve imports and types.

🔧 Suggested adjustment
     - name: Initialize CodeQL
       uses: github/codeql-action/init@v3
       with:
         languages: ${{ matrix.language }}
         queries: security-extended
         config-file: ./.github/codeql/codeql-config.yml
+        build-mode: none
 
-    - name: Autobuild
-      uses: github/codeql-action/autobuild@v3

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread .github/workflows/ci.yml Outdated
Comment thread docs/adr/0002-model-backends.md Outdated
- Extract repeated checkout/pnpm/node/install steps into a reusable composite action (.github/actions/ci-setup)
- Update all four CI jobs (lint, typecheck, test, build) to use the shared action
- Reduces code duplication and simplifies maintenance across CI workflows

Resolves PR review comment: deduplication of CI setup steps
@BjornMelin

Copy link
Copy Markdown
Owner Author

Resolution Summary

Resolved threads: 2 | Files: 2 | Tests: All passing

Changes Made

1. CI workflow deduplication (.github/workflows/ci.yml + new .github/actions/ci-setup/action.yml)

  • ✅ Created composite action .github/actions/ci-setup to encapsulate repeated checkout/pnpm/node/install steps
  • ✅ Updated all four CI jobs (lint, typecheck, test, build) to use the shared action via uses: ./.github/actions/ci-setup
  • ✅ Action accepts inputs for node-version-file, pnpm-version, and fetch-depth with sensible defaults
  • ✅ Net reduction: 8 lines of duplicated config across 4 jobs

2. ADR authentication clarification (docs/adr/0002-model-backends.md line 11)

  • ✅ Updated context section to explicitly state Codex CLI supports both:
    • ChatGPT subscription (Sign in with ChatGPT OAuth)
    • Standard OpenAI API key authentication
  • ✅ Resolves ambiguity about authentication method requirements

Verification

  • Linting: ✅ biome check passes (no diagnostics)
  • Typecheck: ✅ TypeScript strict mode passes
  • Tests: ✅ 62 passing (1 skipped integration test)
  • Commit: 835733b - chore(ci): deduplicate CI setup steps via composite action

Ready to merge once threads are marked resolved.

The composite action requires the repository to be checked out first.
Add actions/checkout@v4 to the beginning of each job's steps before
calling the ci-setup composite action. Remove unused fetch-depth input
from the composite action.

This maintains deduplication of pnpm/node/install while properly
handling the checkout requirement.
@BjornMelin

Copy link
Copy Markdown
Owner Author

Update: Fixed CI workflow - added required actions/checkout@v4 before calling the composite action.

The composite action requires the repository to be checked out first. Structure is now:

  1. Checkout (with fetch-depth: 0)
  2. Composite action (pnpm setup, node setup, install)
  3. Job-specific task (lint/typecheck/test/build)

This maintains full deduplication of the pnpm/node/install steps across all four jobs while properly handling GitHub Actions' checkout requirement.

Commits: 835733b + 2dfa23f

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 64-82: Add job dependencies to enable fail-fast by updating the
GitHub Actions job definition for the "build" job so it depends on the "lint"
and "typecheck" jobs (i.e., add a needs: ["lint","typecheck"] key to the build
job), ensuring "build" only runs after those jobs succeed; keep the existing
runs-on, steps, and conditional pnpm logic intact.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7fda0b0 and 2dfa23f.

📒 Files selected for processing (3)
  • .github/actions/ci-setup/action.yml
  • .github/workflows/ci.yml
  • docs/adr/0002-model-backends.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-22T20:51:16.701Z
Learnt from: BjornMelin
Repo: BjornMelin/codex-toolloop PR: 1
File: docs/specs/060-optional-next-ui.md:107-108
Timestamp: 2026-01-22T20:51:16.701Z
Learning: In the codex-toolloop repository, enforce using double hyphens (--) instead of em dashes (—) in all Markdown documentation. Em dashes are considered an AI antipattern and should be avoided. Apply this guideline across all docs under the docs directory (not just the specified file) and replace any em dashes with -- for consistency and readability.

Applied to files:

  • docs/adr/0002-model-backends.md
🪛 LanguageTool
docs/adr/0002-model-backends.md

[typographical] ~95-~95: Consider using an em dash (—) instead of ‘--’.
Context: ...ection). ## Amendments ### 2026-01-21 -- Codex backend v1 implemented This ADR ...

(TWO_HYPHENS)


[typographical] ~112-~112: Consider using an em dash (—) instead of ‘--’.
Context: ...m/codex/cli/reference/> ### 2026-01-23 -- App-server backend migrated to internal...

(TWO_HYPHENS)


[typographical] ~124-~124: Consider using an em dash (—) instead of ‘--’.
Context: ...types.md- SPEC 023: Protocol workflow --docs/specs/023-codex-app-server-protoc...

(TWO_HYPHENS)

🔍 Remote MCP

Perfect. Now I have enough context to provide a comprehensive review summary. Let me compile the findings.

Key Findings & Review Context

Node.js LTS Status Verification

Node.js 24.11.0 has entered Long-Term Support (LTS) and will continue to receive updates through to the end of April 2028 [::web_search::]. The PR correctly specifies Node.js v24 LTS in ADR-0001 and enforces it via .nvmrc (v24.11), package.json engines constraint (>=24.11.0), and configuration scripts. This is valid and future-proof for production use.

Note on timing: Node.js 24.11.0 officially entered LTS, confirming the PR was created with accurate information (PR created 2026-01-22).


AI SDK with MCP Integration

According to the official AI SDK documentation [::Firecrawl_firecrawl_scrape::], the integration includes:

  • Transport Recommendations: HTTP transport (like StreamableHTTPClientTransport) is recommended for production deployments; stdio transport should only be used for connecting to local servers as it cannot be deployed to production environments

  • MCP Client Initialization: The docs show explicit support for HTTP, SSE, and stdio transports. The PR's approach in ADR-0003 (MCP-tooling) and ADR-0009 (dynamic tool loading) aligns with these recommendations (HTTP/SSE for production, STDIO for local dev).

  • Tool Integration Patterns: The client supports both schema discovery (auto-list all tools) and schema definition (explicit tool definitions with Zod schemas), which matches the PR's dynamic tool-loading design.


Zod v4.3.6 API Changes

Zod v4 moves string format validators (like email, UUID, URL) to the top-level z namespace (e.g., z.email(), z.uuid()) for better tree-shakability; the older method forms (e.g., z.string().email()) are deprecated but still functional [::Context7_query-docs::].

The PR specifies Zod ^4.3.6 and defines multiple schemas in the specs (SPEC-020, SPEC-021, SPEC-030, etc.). Reviewers should verify that any schema code in the implementation uses the new top-level format validators where applicable to align with v4 best practices.


pnpm Workspaces & monorepo Setup

pnpm is excellent for managing monorepos and supports workspace configuration for handling multiple packages efficiently [::Context7_query-docs::].

The PR:

  • Adds pnpm-workspace.yaml and updates package.json with workspaces: ["apps/*", "packages/*"]
  • Introduces pnpm v10.28.1 as the package manager
  • Enforces this via packageManager: "pnpm@10.28.1" in root package.json
  • ADR-0001 documents the decision with supply-chain controls via allowBuilds

The configuration is structurally sound and follows pnpm monorepo best practices.


Turborepo Monorepo Configuration

The PR introduces Turborepo as the task orchestrator with:

  • Root turbo.json configuration
  • Turbo-based build, lint, test, typecheck, and format tasks
  • CI/CD integration via GitHub Actions with Turbo tasks

The pattern is standard and aligns with production monorepo setups combining pnpm workspaces + Turborepo (as documented in SPEC-080).


GitHub Actions Patterns: Composite Actions

The PR adds .github/actions/ci-setup/action.yml as a composite action to reduce duplication across CI jobs. Best practices for GitHub Actions recommend keeping composite actions in .github/actions directory and keeping reusable workflows in .github/workflows [::Firecrawl_firecrawl_search::].

The PR's implementation follows this structure correctly. The composite action encapsulates checkout, pnpm setup, Node.js configuration, and installation—reducing boilerplate in CI workflows (lint, typecheck, test, build jobs all use it).


Testing Framework: Vitest

Vitest is a fast, Vite-native next-generation testing framework offering Jest compatibility, smart watch mode, and out-of-the-box support for ESM, TypeScript, and JSX [::Context7_resolve-library-id::].

ADR-0006 standardizes on Vitest with:

  • pnpm testvitest run
  • pnpm test:watchvitest
  • Test layout under tests/unit/**/*.test.ts and tests/type/**/*.test-d.ts

This aligns with the official Vitest guidance and modern Node.js-first testing practices.


Review Summary

Scope & Quality:

  • Comprehensive foundational documentation (16 ADRs, 9+ specs covering architecture, backends, MCP, security, CI/CD)
  • Product Requirements Document (PRD) establishes clear goals and personas
  • Multiple backend strategies (app-server, exec, SDK) with well-defined interfaces
  • Multi-agent orchestration with typed handoffs and structured outputs

Technical Decisions Validated:

  • ✅ Node.js v24 LTS is production-ready and supported through 2028
  • ✅ AI SDK MCP integration pattern matches official recommendations (HTTP/SSE production, stdio local dev)
  • ✅ Zod v4.3.6 usage aligns with v4 best practices (though code review should verify schema definitions use top-level format validators)
  • ✅ pnpm + Turborepo monorepo setup follows established best practices
  • ✅ GitHub Actions composite action pattern reduces duplication correctly
  • ✅ Vitest standardization is modern and well-supported

Key Areas for Review Attention:

  1. Verify Zod schema code uses v4 top-level format validators (not deprecated method forms)
  2. Confirm MCP client lifecycle management matches the documented close/cleanup patterns
  3. Validate that security guardrails (SPEC-008) are correctly implemented across all backends
  4. Check that protocol artifact regeneration workflow (ADR-0013, SPEC-023) is properly gated in CI

[::Firecrawl_firecrawl_scrape::], [::Context7_query-docs::], [::Context7_resolve-library-id::], [::Firecrawl_firecrawl_search::], [::web_search::]

🔇 Additional comments (10)
.github/actions/ci-setup/action.yml (1)

1-25: Well-structured composite action for CI setup.

The action correctly encapsulates repeated CI setup steps (pnpm, Node.js, install) with sensible defaults. Key positives:

  • Description clearly documents the checkout prerequisite
  • Default pnpm-version: 10.28.1 aligns with package.json#packageManager
  • --frozen-lockfile ensures reproducible installs
  • Explicit shell: bash is correct for composite actions
  • Cache enabled via actions/setup-node for faster subsequent runs
.github/workflows/ci.yml (3)

1-8: LGTM: Workflow triggers and YAML structure.

The quoted "on" key and bracket formatting correctly address previous YAML lint warnings. Push and PR triggers targeting main are appropriate for a standard CI workflow.


10-27: LGTM: Lint job structure.

The job correctly:

  • Uses fetch-depth: 0 to enable Turbo's git-based filtering
  • Leverages the composite action for setup
  • Applies incremental filtering on PRs via --filter=...[origin/${{ github.base_ref }}]

28-82: LGTM: Typecheck, test, and build jobs follow consistent pattern.

All jobs use the same structure (checkout → ci-setup → turbo run with conditional filtering), which is clean and maintainable. The build job additionally runs codex:gen:check to validate generated artifacts.

docs/adr/0002-model-backends.md (6)

7-27: LGTM! Context section is well-structured and complete.

The section clearly establishes the requirements, available integration options, and constraints. The authentication statement on line 11 correctly reflects both ChatGPT subscription OAuth and API key methods, and the integration options are well-differentiated.


28-53: LGTM! Decision section clearly articulates the multi-backend strategy.

The three-backend approach is well-justified, with clear use cases for each:

  • App Server for interactive sessions
  • Exec for deterministic scripting
  • SDK for programmatic control

The workflow-to-backend mapping (lines 48-52) provides concrete guidance for implementation teams.


54-70: LGTM! Alternatives section demonstrates thoughtful decision-making.

Each rejected alternative is documented with clear pros and cons, demonstrating that the multi-backend approach was chosen deliberately to address the limitations of simpler alternatives.


71-77: LGTM! Consequences section provides balanced assessment.

The section honestly acknowledges the implementation complexity while clearly articulating the benefits. The terminal punctuation issue from prior review has been addressed.


78-87: LGTM! Implementation notes provide clear technical guidance.

The section defines concrete implementation requirements:

  • Normalized event model with parsers/adapters
  • Persistence strategy for resumable threads
  • Capability matrix for backend selection

These align well with SPEC 020 referenced in the amendments.


93-124: LGTM! Amendments section provides excellent change history.

The amendments clearly document the evolution of the implementation:

  1. 2026-01-21: Initial v1 implementation with AI SDK community provider
  2. 2026-01-23: Migration to internal schema-driven JSONL-over-stdio client with enhanced runtime safety

The removal of mid-execution injection is consistently documented throughout the file. All referenced external documentation links are accessible, and cross-references to ADR 0013 and SPEC 023 are properly in place.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread .github/workflows/ci.yml
Add needs: ["lint", "typecheck"] to the build job so it only runs
after lint and typecheck jobs succeed. This enables fail-fast by
preventing build from starting if lint or typecheck fails, reducing
wasted compute resources.

Keeps existing runs-on, steps, and conditional pnpm logic intact.
@BjornMelin

Copy link
Copy Markdown
Owner Author

Job dependencies added to CI workflow

Added needs: ["lint", "typecheck"] to the build job (line 66 in .github/workflows/ci.yml).

Benefits:

  • Build job only runs after lint and typecheck succeed
  • Enables fail-fast behavior: if either lint or typecheck fails, build is skipped
  • Reduces wasted compute resources

Structure preserved:

  • Existing runs-on, steps, and conditional pnpm logic remain unchanged
  • Checkout and composite action setup remain the same

Commit: e73ea81

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 1-8: Add explicit workflow-level permissions and a concurrency
key: update the CI workflow (the "name: CI" / "on:" workflow) to include a
permissions block limiting the GITHUB_TOKEN to contents: read and add a
concurrency block (e.g., concurrency: group: ${{ github.workflow }}-${{
github.ref }} cancel-in-progress: true) so runs for the same branch/PR are
deduplicated; place these top-level keys alongside the existing "name" and "on"
entries to enforce least-privilege and prevent stacked runs.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2dfa23f and e73ea81.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml
🧰 Additional context used
🔍 Remote MCP Firecrawl

Additional contextual facts for PR review

  • Node.js 24 (24.11.0 “Krypton”) is in Active LTS and will receive security/maintenance updates through Apr 2028 — confirming ADR/engine constraint is appropriate.,

  • Zod v4 (repo/releases v4.3.6) promotes string-format validators to top-level APIs (e.g., z.email(), z.uuid()), deprecates method forms (z.string().email()), and contains other breaking-migration items (error param, z.core, changed defaults). Code using Zod should be checked for these v4 changes (top-level validators, deprecated methods, changed error handling).,

  • pnpm v10 (release 10.28.1) includes workspace/security features the PR references:

    • allowBuilds: explicit map to allow/disallow install/build script execution (replaces older onlyBuiltDependencies/ignoredBuiltDependencies).
    • supply-chain settings (minimumReleaseAge, trustPolicy, blockExoticSubdeps, etc.) and workspace-oriented config (pnpm-workspace.yaml) are available and relevant to ADRs in the PR — verify repo pnpm-workspace.yaml and CI usage align with these settings.,
  • Vitest is ESM-first, TypeScript- and Vite-integrated test runner (recommended and documented in ADRs). Key points: ESM/TS support, run vs watch modes (CI detection), concurrency/threading options, and project configs — verify CI/test scripts and vite/vitest config follow Vitest recommendations.,

Sources used: Firecrawl web searches above.

🔇 Additional comments (4)
.github/workflows/ci.yml (4)

10-27: LGTM!

The lint job structure is solid:

  • fetch-depth: 0 enables Turborepo to detect changes correctly.
  • Composite action usage reduces duplication.
  • Conditional Turbo filter (--filter=...[origin/${{ github.base_ref }}]) for PRs optimizes CI by only running affected tasks.
  • Turbo remote caching credentials are properly sourced from vars/secrets.

28-45: Consistent pattern with lint job.

The typecheck job correctly follows the same structure, ensuring maintainability.


46-63: LGTM!

The test job runs in parallel with lint/typecheck (no needs dependency), which is a valid design choice—it provides faster feedback by showing all failures at once rather than incrementally. If tests are expensive and you want to avoid running them on PRs that fail lint/typecheck, you could add dependencies, but parallel execution is reasonable here.


64-83: LGTM!

The build job correctly:

  • Depends on lint and typecheck for fail-fast behavior, reducing wasted compute.
  • Combines build and codex:gen:check tasks in a single Turbo invocation for efficiency.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread .github/workflows/ci.yml
Add workflow-level security and reliability improvements:
- permissions.contents: read - Apply least-privilege principle, restricting
  GITHUB_TOKEN to read-only access to repository contents (required for
  checkout)
- concurrency - Deduplicate and cancel in-progress runs for the same
  branch/PR to prevent stacked runs and reduce unnecessary CI minutes

Concurrency cancellation is enabled for PRs to prioritize latest changes,
while main branch runs are not cancelled.
@BjornMelin

Copy link
Copy Markdown
Owner Author

Explicit permissions and concurrency control added

Added two security and reliability improvements to the CI workflow (lines 9-14):

1. Permissions (Least-Privilege)

permissions:
  contents: read
  • Restricts GITHUB_TOKEN to read-only access to repository contents
  • Follows the principle of least privilege—only grants permissions actually needed (for checkout)
  • Protects against token misuse if workflow is compromised

2. Concurrency Control

concurrency:
  group: ci-${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
  • Deduplicates runs: Same branch/PR won't stack multiple runs
  • Cancels in-progress PR runs: When new commits are pushed to a PR, previous runs are cancelled (prioritizes latest changes)
  • Preserves main runs: Main branch runs are not cancelled (no in-progress cancellation on push)
  • Reduces CI minutes: Prevents wasted compute on stacked runs

Result:

  • Tighter security posture
  • Better reliability and faster feedback
  • Reduced CI costs

Commit: 54dc715

@BjornMelin BjornMelin merged commit cc14415 into main Jan 25, 2026
8 checks passed
@BjornMelin BjornMelin deleted the chore/init-core-docs branch January 25, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants