Repository navigation
refactor(cli): move the adapter contract, stub and Hermes into packages/seats - #2672
lilyshen0722 wants to merge 1 commit into
Conversation
…es/seats D62 extract-in-place, PR 1. No behaviour change. packages/seats (private, Apache-2.0) now holds the adapter contract and registry, the upstream-refusal naming, spawn observation, the seat-model check and two adapters. Hermes becomes a factory that takes Commonly's policy as a host object, so the package imports nothing from Commonly. The old cli paths re-export or bind it, so no caller and no existing test changes. The CLI ships the package as a bundled file: dependency. Two publish fixes come with it: - npm pack on an uninstalled tree exits 0 and silently drops the bundle. cli's prepack now refuses that case. - npm-publish installs before its compare step and triggers on packages/seats. seats-package.test.mjs covers: - the import boundary (mutation-checked); - that the tarball carries every package file (mutation-checked); - that the registry serves the bound adapters; - the neutral defaults. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Package contracts and shared helpers packages/seats/package.json, packages/seats/README.md, packages/seats/src/contract.js, packages/seats/src/seat-model.js, packages/seats/src/runtime-observation.js, packages/seats/src/upstream-refusal.js, packages/seats/src/index.js |
The package defines adapter and host contracts, registry and provider-key helpers, seat-model validation, runtime observation, and upstream-refusal helpers. Its barrel file exports these helpers. |
Stub and Hermes adapters packages/seats/src/adapters/stub.js, packages/seats/src/adapters/hermes.js |
The package adds the stub adapter and Hermes adapter. The Hermes adapter detects supported installations, validates its environment, processes JSONL events, and manages the subprocess. |
CLI integration and package distribution cli/src/lib/adapters/*, cli/src/lib/runtime-observation.js, cli/src/lib/upstream-refusal.js, cli/package.json, cli/scripts/require-bundled-seats.mjs, cli/__tests__/seats-package.test.mjs, .github/workflows/npm-publish.yml |
The CLI re-exports implementations from the package and adds it as a bundled local dependency. A prepack check and tests cover package imports, adapter wiring, and tarball contents. The publish workflow tracks package changes and installs dependencies before comparison. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Refactor
Possibly related PRs
- Team-Commonly/commonly#2582: Changes Hermes 0.22 behavior that this PR relocates into the seats package.
- Team-Commonly/commonly#2610: Changes the publish workflow’s packed-artifact comparison that this PR updates.
- Team-Commonly/commonly#2663: Changes the seat-model requirement that this PR extracts into the package helper.
Merge Risk: 
9d3e5
-
This is an extraction of existing seat adapter code into a private package, with the CLI bundling it and the publish workflow tracking it. No actionable merge-blocking risk was identified. Normal CI and the author's pending rebase onto
#2582remain the remaining checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: moving the adapter contract, stub, and Hermes adapter into packages/seats. Description check The description is directly related to the changeset and explains the package extraction, CLI binding, bundling, workflow updates, tests, and reported validation results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
PASS at 9d3e5ec for the workflow and the prepack guard. Rhea's security gate is separate.
- OIDC only: the publish job still has no
NODE_AUTH_TOKENand nosecrets.reference. It keepsid-token: writeandnpm publish --provenance. The movednpm ci --ignore-scriptsstep is read-only and runs before the branch guard. Thenpm-releaseenvironment still gates it to main. - Guard: with
@commonlyai/seatsinstalled,npm pack --dry-runin cli/ includesnode_modules/@commonlyai/seats/*and exits 0. With that link removed, the same command exits 1 fromprepackwith the "runnpm ciin cli/" message. - cli
seats-package.test.mjs: 6/6 at this head.
Not re-measured: the claim that npm pack on an uninstalled tree exits 0 without the guard. The guard does not depend on it.
Written by Sable, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: PASS
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
SECURITY gate for #2672 (refactor(cli): move the adapter contract, stub and Hermes into packages/seats) at 9d3e5ec. Head check: OPEN, not draft. Touches .github/workflows/npm-publish.yml, so this is the Tier 2 publish-workflow review.
Checked, both asked for:
- OIDC only, no token. The publish workflow has no secrets reference and no NODE_AUTH_TOKEN or _authToken (grep of the whole file at 9d3e5ec). The new Install step (
npm ci --ignore-scripts) has no env. Publish still runsnpm publish --provenance --access publicwith no token, and the job keepsid-token: writeandcontents: writeonly at job level. The trigger is push to main plus workflow_dispatch, with no pull_request, so untrusted PR code does not run here. Dispatch from a non-main ref is refused by the cmp step, and the npm-release environment binds it to main. - The guard fails on a missing bundled dependency. In a fixture (cli with a file: dep and bundleDependencies),
npm pack --dry-runwith no install runs prepack and exits non-zero with "missing" from require-bundled-seats.mjs. Afternpm install --ignore-scripts, the symlinked node_modules/@commonlyai/seats packs in as 4 files, so the guard's existence check tracks a correct bundle.
Moving the install before the compare is a real fix. Before it, a compare on a tree with no node_modules could publish a CLI without its seats. node_modules is gitignored (.gitignore:10), so the git clean -fdq after the diff does not delete the install before publish.
Conditions and notes, none blocking:
--ignore-scriptsis load-bearing. The job now holds id-token: write from the start and runs npm ci on every push that touches cli, including the skip path. That was already true of the publish path. Keep the flag. Dropping it would let an install script read the OIDC request env.- The guard checks that the seats manifest exists, not that its contents match. That holds for the symlink layout this PR uses. If someone later vendors a copy or changes the dep to a registry version, re-check the guard.
- I did not inspect cli/package-lock.json integrity beyond the diff. That is the cli lane's review.
Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015
GATE: PASS at 9d3e5ec — the publish workflow stays OIDC-only with no token, the prepack guard fails when the bundled seats package is missing, and the install before compare is what keeps the tarball complete. Conditions: keep --ignore-scripts on the install step, and re-check the guard if the seats dependency changes from a symlink.
|
BYO walk (rule 58) at head
|
D62 extract-in-place, PR 1 of 5. No behaviour change.
What moves
packages/seatsis new: private,type: module, Apache-2.0. It holds:contract.js: the ADR-005 adapter contract in JSDoc, pluscreateRegistryandproviderKeyEnvNamesupstream-refusal.js: moved wholeruntime-observation.js:controlsFromArgumentsandobserveSpawnonly.createRuntimeReporterposts to a Commonly instance, so it stays in the CLI.seat-model.js:requireSeatModel(environment, name, required = false). TheCOMMONLY_REQUIRE_SEAT_MODELread stays in the CLI.adapters/stub.js: moved wholeadapters/hermes.js: now the factorycreateHermesAdapter(host). The host suppliesmemoryPreamble,effectiveTrust,seatModelRequiredandprofileRoot, and each has a neutral default.The old CLI paths become one-line re-exports, and
cli/src/lib/adapters/hermes.jsbinds the factory to Commonly's policy. The run loop, the registry, the other 7 adapters and every existing test are unchanged.How it ships without publishing
cli/package.jsonadds"@commonlyai/seats": "file:../packages/seats"and lists it inbundleDependencies, so the package ships inside the@commonlyai/clitarball.Measured with npm 10.9 and 11.21:
npm ci,npm packcarries all 8 package files;npm packexits 0 and silently leaves them out.Two changes follow from that second result:
cligets aprepackguard (scripts/require-bundled-seats.mjs) that refuses to pack when the bundle isn't installed;npm-publish.ymlrunsnpm ci --ignore-scriptsbefore its compare step, not only before publish. Before this change the compare packed an uninstalled tree, so every run would have looked different from npm and triggered a publish. The workflow also now triggers onpackages/seats/**.Proof
New
cli/__tests__/seats-package.test.mjs(6 tests):import '../../../cli/src/lib/environment.js'fails it.npm pack --dry-runlists every package source file undernode_modules/@commonlyai/seats/. Mutation check: droppingbundleDependenciesfails it.Local results:
adapters.hermes,upstream-refusal,runtime-observation,attachandadapter-failure-scrub: 131/131, both withCOMMONLY_REQUIRE_SEAT_MODELunset and with it set to 1.backend/node_modules, which this worktree doesn't have and CI installs.npm run lintis clean.Jest resolves the symlinked
file:dependency without any config change.Walk: SRE will run the issued public BYO command once at this head, and the result goes here before any gate ask.
Ordering
This conflicts with #2582 in the Hermes adapter. #2582 should land first. I'll rebase this PR and carry its
--in/ 0.22 range change intopackages/seats/src/adapters/hermes.js.Design and import inventory: the D62 design note, kept outside the repo.
🤖 Generated with Claude Code
https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu