Repository navigation
CI, a type check that passes, a changelog - #4
Merged
Merged
Conversation
With no build step, nothing checks types unless something asks: `tsx` transpiles and throws the types away, which is why a strict `tsc --noEmit` over the sources found 17 errors that had been sitting in running code. `yarn typecheck` is that ask, and it carries the compiler flags in place of the `tsconfig.json` this repository deliberately does not have — until now those flags existed only inside the audit document. The errors were real, if mostly harmless. `src/client.ts` read `status` and `artifact` off stream payloads as though a protobuf message field were ever required; a status update without a message would have thrown in the printer rather than printing nothing. Its `default:` branch reached for `$case` on an exhausted union, so the one line meant to describe an unrecognised payload could not have described it. `src/acp/client.ts` declared its adapters as `ChildProcessWithoutNullStreams` while spawning them with stderr inherited — two pipes, not three — and read fields off the `initialize` response the SDK hands back as `unknown`. Verified: `yarn typecheck` reports nothing across `src/`, and `yarn client` against the Revisor stub still walks the documented cycle — `SUBMITTED → INPUT_REQUIRED → WORKING → artifact → COMPLETED` — with every event rendered as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two workflows, and neither introduces a thing to maintain. `ci.yml` installs with `--immutable` and runs `yarn typecheck` and `yarn test`. Corepack is enabled before the install, because that is what resolves the Yarn version pinned in `packageManager`, and setup-node's cache is deliberately not used: it reaches for the Yarn 1 shim on the runner and asks it about a Berry cache folder that is global here anyway. `docs.yml` publishes `docs/` to GitHub Pages through `actions/jekyll-build-pages` — the same Jekyll that Pages runs by itself. So the site is the Markdown already in the repository rather than a second copy of it, and `docs/_config.yml` is four settings: a theme, and `jekyll-relative-links` so that `[permissions](permissions.md)` keeps working both in the repository and on the rendered site. It runs on pushes to `main` that touch `docs/`, with a `pages` concurrency group so a queued deployment waits rather than replacing what is live. One thing the workflow cannot do for itself: Pages has to be set to "GitHub Actions" as its source in the repository settings, or the deploy step fails with nothing to deploy to. Verified by parsing both workflows and the Jekyll config, and by running `yarn typecheck` through the script CI calls. `yarn test` is still the `npm init` stub on this branch and fails as it always has; it becomes real with the unit-test branch, which is why CI calls it now rather than later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The document was a snapshot of one review and has been read since as a plan, which left no way to tell an open finding from one that was answered three commits ago. Each finding now carries a **Status** line, and a summary near the top says which are closed, which are partly closed and which are untouched. The findings themselves are left exactly as they were written. The value of an audit is that it records what was true at a moment; editing a finding away once it is fixed loses the reason anyone should keep checking that it stays fixed. So `F01` still describes a context id becoming a trusted root, and the line under it says that directories now come from `mkdtemp`. Ten are closed, two are partly closed — F18, whose two MCP deadlines stopped throwing, and F21, which has the typecheck and CI but not the smoke exit codes — and eleven are open. The open ones include the group the implementation order deliberately keeps together, F04–F07: they share the adapter registry, and fixing one alone moves the bug rather than removing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A2A was the only one with a specification link anywhere: the README, the docs index, the A2A page and the audit all pointed at it, while ACP and MCP were named repeatedly and never sourced. That is backwards for the two that are harder to look up — A2A is searchable by name, and "ACP" is not. Each of the three now sits next to the version this repository actually speaks, read off the handshakes rather than the package names, because the two disagree in a way that misleads: `@agentclientprotocol/sdk` is 1.4.0 while the ACP protocol version it negotiates is `1`, and `@modelcontextprotocol/server` is 2.0.0 while the MCP version our `initialize` answers with is `2025-06-18`. The bridge pages now open the way `docs/a2a.md` already did, with the specification and the SDK on the first line. All five URLs checked with curl: 200. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixteen commits in and the only way to learn what changed between two points was to read the log, which records what each commit did and not what any of it added up to. Two squash merges in particular hid a lot: `3ddd7d4` and `109fb67` are one line each in `git log` and ten audit findings between them. `CHANGELOG.md` at the root, Keep a Changelog format, with the audit's `F01`/`F09` numbering carried through so an entry can be traced to the finding that provoked it and the evidence recorded there. One section, `0.1.0`, because nothing has been released yet and everything here is what that version will contain. The isolation work sits under `Security` rather than `Fixed`, because a sandbox that could be steered onto the repository root is not a bug in the ordinary sense. `AGENTS.md` says who updates it and when — a change to behaviour, configuration or the protocol surface, in the commit that makes it, not afterwards from memory; refactors and documentation edits do not qualify, or the file becomes the log again. And once 0.1.0 has gone out, new entries open an `## [Unreleased]` above it rather than editing what shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Written a day ago against versions that were already one to three majors behind, because the ones in every example online are the ones from whenever that example was written. Checked against each action's releases rather than from memory: | Action | Was | Now | Latest release | |---|---|---|---| | `actions/checkout` | v4 | v7 | v7.0.1, 2026-07-20 | | `actions/setup-node` | v4 | v7 | v7.0.0, 2026-07-14 | | `actions/configure-pages` | v5 | v6 | v6.0.0, 2026-03-25 | | `actions/upload-pages-artifact` | v3 | v5 | v5.0.0, 2026-04-10 | | `actions/deploy-pages` | v4 | v5 | v5.0.1, 2026-09-01 | | `actions/jekyll-build-pages` | v1 | v1 | v1.0.13 — no major since 2024 | Nothing we use changed shape across those majors: the two big ones are ESM migrations and a move to the Node 24 runtime, and `configure-pages`' breaking change was to `static_site_generator: next`, an input this does not pass. The three Pages actions are bumped together and now say so in a comment — they agree on an artifact format, so raising one alone is the usual way a Pages job starts failing. `node-version` stays at `'24'`, which is the active LTS until 20 October 2026; the comment now says that, and why it is a literal rather than `lts/*` — a version that changes itself on a date nobody remembers is not what CI is for. Both workflows still parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five commits: two workflows, the type check they run and the errors it found, the audit marked
up with what has been done, specification links for the two protocols that had none, and a
changelog.
Rebased onto
mainafter the unit tests landed, sosrc/acp/client.tshere is the refactoredone — the type fixes were re-applied to it rather than merged line by line.
What is in it
8570f1byarn typecheck, and the 17 type errors it found in running code617d266.github/workflows/ci.ymlanddocs.yml,docs/_config.yml1403d93e9e11b7336f43eCHANGELOG.md, and the rule inAGENTS.mdfor keeping itCI
yarn install --immutable→yarn typecheck→yarn test, on every push and pull request.Corepack is enabled before the install because that is what resolves the Yarn version pinned in
packageManager;setup-node's cache is deliberately not used, since it reaches for the Yarn 1shim and asks it about a Berry cache folder that is global here anyway.
Docs
actions/jekyll-build-pagesoverdocs/— the same Jekyll that Pages runs by itself, so the siteis the Markdown already in the repository rather than a second copy of it.
docs/_config.ymlisfour settings, the load-bearing one being
jekyll-relative-links, so that[permissions](permissions.md)keeps working both in the repository and on the rendered site.Pages is already set to "GitHub Actions" on this repository, so the first push to
mainthattouches
docs/will deploy.The type errors were real
src/client.tsreadstatusandartifactoff stream payloads as though a protobuf messagefield were ever required — a status update without a message would have thrown in the printer
rather than printing nothing — and its
default:branch reached for$caseon an exhaustedunion, so the one line meant to describe an unrecognised payload could not have described it.
src/acp/client.tsdeclared adapters asChildProcessWithoutNullStreamswhile spawning them withstderr inherited, and read fields off an
initializeresponse the SDK hands back asunknown.Checks
yarn typecheck— clean acrosssrc/, which is now the CI-enforced baselineyarn test— 288 passed, 13 filesyarn clientagainst the Revisor stub — the documented cycle, unchanged, with every eventrendered as before
Note for the merge
The changelog has a single
## [0.1.0] — 2026-09-21section, since nothing has been releasedyet and everything on
mainis what that version will contain. Once it goes out, new entriesopen an
## [Unreleased]above it rather than editing what shipped —AGENTS.mdsays so.🤖 Generated with Claude Code