Conversation
🦋 Changeset detectedLatest commit: c4e7771 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (35)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64ce981e5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // gives Check C teeth: tampered content whose manifest still carries | ||
| // the original integrity now fails here. | ||
| const recomputed = computeDirIntegrity(stagingDir, Object.keys(buildManifest.assets)) | ||
| const recomputed = computeDirIntegrity(stagingDir, Object.keys(archive.fileHashes)) |
There was a problem hiding this comment.
For a valid 0.2 archive containing a binary supplementary file, this call reaches computeDirIntegrity, which reads every declared file as UTF-8 text before hashing and rebuilding the tar. Invalid UTF-8 bytes are replaced during decoding, so the recomputed file hashes and archive integrity no longer match the verified files map; handleMiss then reports an integrity failure and refuses to cache/install the archive. This affects the checked-in 0.2 fixture's assets/logo.bin; recomputation must preserve Uint8Array bytes.
Useful? React with 👍 / 👎.
| return { | ||
| ok: true, | ||
| data: { buildManifest: validated, innerArchiveBytes: new Uint8Array(innerEntry.data) }, | ||
| data: { manifest: manifestResult.data, innerArchiveBytes: innerBytes }, |
There was a problem hiding this comment.
This public result shape no longer matches the documented Node smoke test: scripts/smoke/protocol-node.mjs still reads result.data.buildManifest for successful parses and result.errors for failures. After building the package, running the advertised node scripts/smoke/protocol-node.mjs therefore fails both parseFacetArchive checks instead of validating the published Node bundle. Update that script to use the tagged data.manifest.manifest and failure result shapes.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { type AssetType, type ValidationError, validateAssetName } from '@agent-facets/common' | ||
| import { planArchiveEntries } from '../build/archive-plan.ts' | ||
| import { computeContentHash, INNER_ARCHIVE_NAME, parseFacetArchive, parseInnerArchive } from '../build/content-hash.ts' | ||
| import { detectNamingCollisions } from '../build/detect-collisions.ts' | ||
| import { validateRawTarEntries } from '../build/tar-headers.ts' | ||
| import { validateContentFiles } from '../build/validate-content.ts' | ||
| import { validateCompactFacets } from '../build/validate-facets.ts' | ||
| import { FACET_MANIFEST_FILE, resolvePromptsFromMap, validateLegacyFacetManifest } from '../loaders/facet.ts' | ||
| import type { BuildManifest } from '../schemas/build-manifest.ts' | ||
| import { | ||
| FACET_MANIFEST_FILE, | ||
| resolvePromptsFromMap, | ||
| validateFacetManifest, | ||
| validateLegacyFacetManifest, | ||
| } from '../loaders/facet.ts' | ||
| import type { CurrentBuildManifest, LegacyBuildManifest } from '../schemas/build-manifest.ts' | ||
| import { FACET_ARCHIVE_VERSION, LEGACY_FACET_ARCHIVE_VERSION } from '../schemas/build-manifest.ts' | ||
| import type { FacetManifest } from '../schemas/facet-manifest.ts' | ||
| import type { LegacyFacetManifest } from '../schemas/facet-manifest-legacy.ts' | ||
| import type { AssetIntegrityFailure, FacetIntegrityFailure } from './types.ts' | ||
| import { verifyHash } from './verify.ts' | ||
|
|
There was a problem hiding this comment.
This PR removes VerifiedArchive from @agent-facets/protocol's public exports (replaced by VerifiedFacetArchive) and changes the validateFacetArchive failure branch from { ok: false, errors: ValidationError[] } to { ok: false, failure: ArchiveVerificationFailure }. The closely coupled registry at packages/v0/api/src/routes/facets/publish/verify.ts imports VerifiedArchive on line 12 (now gone), calls classifyError(result.errors) on line 65 where errors is now failure, and accesses v.assets in toVerifiedArtifact (line 103–115) which exists only on the 0.1 discriminant of the new VerifiedFacetArchive union. When the protocol package version bumps, the registry's upload-verification gateway will fail to compile.
64ce981 to
7e815ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e815ef96d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| byKey.set(key, name) | ||
| entries.push({ path: name }) | ||
| } |
There was a problem hiding this comment.
For a 0.2 archive whose entries and files hashes are correct but whose inner tar entries are not lexicographically ordered, this validator succeeds because it records paths without checking their sequence. The registry download path then extracts it and computeDirIntegrity sorts those paths before rebuilding the tar, so the reconstructed hash differs from the manifest's hash and installation fails after verification succeeded. Enforce the canonical ordering here (or preserve the verified tar bytes for the cache check) so accepted archives are installable.
Useful? React with 👍 / 👎.
2dbd6c3 to
fef9bb7
Compare
7e815ef to
2b854bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b854bf033
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const typeflagByte = tarBytes[offset + TYPEFLAG_OFFSET] ?? 0 | ||
| const typeflag = typeflagByte === 0 ? '0' : String.fromCharCode(typeflagByte) | ||
| const name = readField(tarBytes, offset + NAME_OFFSET, NAME_LENGTH) | ||
| const prefix = readField(tarBytes, offset + PREFIX_OFFSET, PREFIX_LENGTH) | ||
| const size = readOctalSize(tarBytes, offset + SIZE_OFFSET) |
There was a problem hiding this comment.
A tar whose header checksum has been corrupted while its name, size, and payload remain intact is accepted by this raw validator (for example, mutating byte 148 of valid-0.2.facet still returns ok: true). The checksum is the tar format's structural integrity check, so accepting it means parseFacetArchive can bless malformed archives that conforming tar consumers may reject. Parse and verify the checksum field for each header, returning tar-malformed on a mismatch.
Useful? React with 👍 / 👎.
fef9bb7 to
6a969ca
Compare
2b854bf to
3526a29
Compare
6a969ca to
a3ab253
Compare
3526a29 to
93a9c9b
Compare
a3ab253 to
1ba7c8d
Compare
009140b to
e2b15b8
Compare
1ba7c8d to
b6ee73c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2b15b8166
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function readField(bytes: Uint8Array, offset: number, length: number): string { | ||
| let end = offset | ||
| const max = offset + length | ||
| while (end < max && bytes[end] !== 0) end++ | ||
| return new TextDecoder().decode(bytes.subarray(offset, end)) | ||
| } |
There was a problem hiding this comment.
When an inner entry name contains invalid UTF-8 (for example files/\xff.bin), the default TextDecoder silently replaces the bad byte with �. A crafted 0.2 archive can declare that replacement-character path in facet.json and the files map, so verification succeeds even though the raw header path is not the path later extracted. The cache recomputation then rebuilds the tar using UTF-8 bytes for � rather than the original invalid byte, producing a different integrity hash and making installation fail after successful validation. Decode header fields fatally (and return tar-malformed) so accepted paths round-trip losslessly.
Useful? React with 👍 / 👎.
e2b15b8 to
54323fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54323fe4ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const typeflagByte = tarBytes[offset + TYPEFLAG_OFFSET] ?? 0 | ||
| const typeflag = typeflagByte === 0 ? '0' : String.fromCharCode(typeflagByte) | ||
| const name = readFieldFatal(tarBytes, offset + NAME_OFFSET, NAME_LENGTH) | ||
| const prefix = readFieldFatal(tarBytes, offset + PREFIX_OFFSET, PREFIX_LENGTH) | ||
| const size = readOctalSize(tarBytes, offset + SIZE_OFFSET) |
There was a problem hiding this comment.
An archive whose entries and file hashes are valid but whose inner tar uses a nonzero mtime (or different mode/uid/gid) can pass this validator after its manifest integrity is computed from those raw bytes, because only type, name, prefix, and size are inspected. The registry miss path then extracts the files and reconstructs them with DETERMINISTIC_ATTRS, so its computed integrity differs from the accepted archive's manifest and installation fails at Check C. Validate the remaining deterministic header fields (and zero-filled name-field tails), or preserve the verified inner-tar bytes for cache integrity computation.
AGENTS.md reference: packages/protocol/AGENTS.md:L33-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 54323fe. Configure here.
| throw new Error(`unreachable archive failure: ${JSON.stringify(unreachable)}`) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated archive failure formatters
Low Severity
describeArchiveFailure and describeVerificationFailure both switch on the full ArchiveVerificationFailure union with nearly the same case logic. Keeping two copies raises the odds that a new failure code or message change is updated in only one path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 54323fe. Configure here.
e4de25e to
8bdf957
Compare
Merge activity
|
… structured `ArchiveVerificationFailure` to the consumer bridge
8bdf957 to
c4e7771
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4e7771ce1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (offset === tarBytes.length) { | ||
| // Tar without an end-of-archive marker. nanotar tolerates this; a | ||
| // canonical builder always emits the marker, but the bytes are | ||
| // unambiguous, so accept. | ||
| break |
There was a problem hiding this comment.
When a 0.2 inner tar ends exactly after its last payload block, this accepts it even though assembleTar always emits terminal zero blocks. The archive can then pass validateFacetArchive, but registry installation extracts it and computeDirIntegrity rebuilds a terminated canonical tar, yielding a different integrity hash and rejecting the previously verified archive at Check C. Return tar-malformed here (at least for canonical inner archives) so accepted archives remain reproducible. Tar bytes are part of the hash contract.
AGENTS.md reference: packages/protocol/AGENTS.md:L33-L37
Useful? React with 👍 / 👎.
This PR was auto-generated by the release workflow. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @agent-facets/protocol@0.29.0 ### Minor Changes - [#437](#437) [`5a837c5`](5a837c5) Thanks [@eXamadeus](https://github.com/eXamadeus)! - **Consumer support for archive format `0.2` (pre-1.0 breaking minor).** The protocol now verifies both legacy `0.1` and current `0.2` `.facet` archives with strict, exact `facetVersion` dispatch and no fallback between versions. This is the consumer-first release: verification support ships before any producer emits `0.2`. **Breaking API — `validateFacetArchive`.** The result shape is now `{ ok: true; data: VerifiedFacetArchive } | { ok: false; failure: ArchiveVerificationFailure }`. The previous `{ ok: false; errors: ValidationError[] }` arm is replaced by a single tagged `failure`. The success payload type `VerifiedArchive` is renamed to `VerifiedFacetArchive` and is now a discriminated union on `archiveVersion`: the legacy `0.1` arm keeps the flat `assets: VerifiedAsset[]` list, while the current `0.2` arm exposes `entries: VerifiedEntry[]` (each tagged `manifest` \| `primary-asset` \| `skill-companion` \| `archive-only`). Consumers that read `.assets` unconditionally should migrate to the version-agnostic helpers `listVerifiedFiles(archive)` and `verifiedFileHashes(archive)`. **Structured failures.** `ArchiveVerificationFailure` is a tagged union (`container`, `invalid-json`, `duplicate-members`, `unsupported-facet-version`, `schema-violation`, `decompression`, `integrity`, `entry-integrity`, `validation`); classify on `failure.code` rather than parsing messages. No expected failure mode throws. **New public API.** `VerifiedFacetArchive`, `VerifiedEntry`, `ArchiveVerificationFailure`, `ValidateFacetArchiveResult`, `listVerifiedFiles`, `verifiedFileHashes`; versioned build-manifest and lockfile schemas plus their exact-dispatch parsers `parseBuildManifestDocument` and `parseLockfileDocument`; the shared archive plan (`planArchiveEntries`, `validateSupplementaryPath`, `portableCollisionKey`); strict raw tar-header validation (`validateRawTarEntries`, `RawTarValidationOptions`); and the archive-format constants `FACET_ARCHIVE_VERSION` (`0.2`), `LEGACY_FACET_ARCHIVE_VERSION` (`0.1`), and `SUPPORTED_FACET_VERSIONS`. `parseFacetArchive` now returns a version-tagged parsed build manifest and a structured `failure`. **Transitional exports retained.** `BuildManifestSchema`/`BuildManifest`, `LockfileSchema`/`Lockfile`, and `LOCKFILE_VERSION` (which equals the legacy value `1`, not the current `0.2`) remain exported and `@deprecated` for the compatibility window; they are removed once the engine lockfile-migration and producer work lands. Prefer the versioned parsers and `CURRENT_LOCKFILE_VERSION` in new code. This release intentionally carries **no** `@agent-facets/adapter` or `agent-facets` (CLI) version bump: the adapter API `0.0`→`0.1` cutover and the CLI `0.2` producer ship in later, separately gated releases. Other implementations of the spec (e.g. the registry) adopt this published package to gain dual-format verification. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > No application code changes—only version, lockfile, and changelog updates from the release workflow. > > **Overview** > Automated **Changesets** release PR that bumps **`@agent-facets/protocol`** from **0.25.0** to **0.29.0**, updates **`bun.lock`**, and appends the **0.29.0** section to **`packages/protocol/CHANGELOG.md`**. > > It **removes** the consumed changeset **`.changeset/protocol-0-2-consumer-support.md`** so the release notes are not applied twice. There are **no runtime or source changes** in this diff—only version and changelog metadata ahead of npm publish. > > The published **0.29.0** minor (from [#437](#437)) documents consumer support for facet archive **`0.2`**, breaking **`validateFacetArchive`** / **`VerifiedFacetArchive`** APIs, structured **`ArchiveVerificationFailure`** codes, and new verification helpers and versioned manifest/lockfile parsers; adapter and CLI bumps are explicitly deferred to later releases. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f30ba6d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 participant
Why
Archive verification needs to support both the legacy
0.1format and the new0.2format, which introduces supplementary (non-asset) files, afileshash map instead ofassets, and stricter membership rules derived from the shared archive plan. The consumer bridge lets the CLI and engine accept both formats while no producer yet emits0.2.Details
Strict raw tar-header validation (
tar-headers.ts) is now applied to both the outer container and the inner archive before any path-keyed structure is built. This rejects duplicate paths, portable aliases (case/Unicode collisions), non-regular entry types (symlinks, directories, PAX/GNU headers), non-canonical names (traversal, absolute, backslash, ustar prefix), and non-zero bytes after the end-of-archive marker. After this validation passes, nanotar's extraction is guaranteed to agree with the validated entry list.parseFacetArchivenow performs raw-header validation before selecting either outer entry, enforces an exact two-entry set (build-manifest.json+archive.tar.gz), and delegates toparseBuildManifestDocumentfor versioned manifest parsing with duplicate-JSON-member rejection and structuredunsupported-facet-versionfailures. A malformed0.2manifest is never retried under0.1rules.validateFacetArchivenow returnsValidateFacetArchiveResult(replacingValidated<VerifiedArchive>) with a taggedArchiveVerificationFailureunion covering every expected failure mode:container,invalid-json,duplicate-members,unsupported-facet-version,schema-violation,decompression,integrity,entry-integrity, andvalidation. The success branch returns aVerifiedFacetArchivetagged by exactarchiveVersion(0.1or0.2).Version-dispatched content verification splits into two isolated paths:
0.1(legacy, frozen): per-asset hash reconciliation againstassets, legacy facet-manifest schema, conventional-path outer-exclusivity allowlist, legacy content rules.0.2(current): embedded manifest validated under current rules; expected membership derived fromplanArchiveEntries(the shared archive plan, not the attacker-controlled build manifest); exact three-way set equality among expected paths, observed entries, andfileshash-map keys; entries returned as taggedVerifiedEntryvalues with supplementary content kept as opaque bytes and only primary assets decoded as text.listVerifiedFilesandverifiedFileHashesprovide uniform extraction views over either format so callers (registry download, cache staging) don't branch per version.cachePutVerifiednow accepts{ integrity, fileHashes }instead of aBuildManifest, making it version-neutral. All call sites pass the version-selected hash map explicitly.downloadAndExtractFacetnow returnsDownloadedArchiveInfo({ integrity, fileHashes }) and surfacesunsupported-facet-versionas a typedUNSUPPORTED_ARCHIVEregistry error so the CLI can render actionable upgrade guidance instead of a generic network error.CLI error rendering adds
describeVerificationFailurefor structured archive failures inpublish, andFailureBlockandtranslateEngineRegistryErrorhandleUNSUPPORTED_ARCHIVEwith upgrade guidance.Immutable fixtures (
valid-0.1.facet,valid-0.2.facet) are checked in as compatibility anchors. They pin that future consumers keep accepting today's bytes — regenerating them signals a breaking format change.Verification
The full test suite covers: immutable fixture round-trips for both versions, raw-header attacks (duplicates, aliases, all non-regular typeflags, traversal, absolute paths, ustar prefix, trailing data), tampered inner archives, missing/extra entries, hash mismatches for both primary and supplementary files, duplicate JSON members in both the build manifest and embedded
facet.json, unsupported version dispatch, and the no-legacy-fallback invariant for malformed0.2archives. CI runs all protocol and engine consumer tests.Note
High Risk
Touches archive integrity and verification at a supply-chain boundary with a breaking public API and extensive failure-mode behavior; regressions could accept bad archives or reject valid ones.
Overview
Ships a protocol-only, consumer-first release: verifiers accept legacy
0.1and current0.2.facetarchives with exactfacetVersiondispatch and no cross-version fallback.Breaking
validateFacetArchiveAPI. Failures are a taggedArchiveVerificationFailure(failure.code) instead of a flaterrors[]. Success returnsVerifiedFacetArchivediscriminated onarchiveVersion(0.1flatassetsvs0.2classifiedentries). New helperslistVerifiedFiles/verifiedFileHashesavoid version branching at call sites.Stricter parsing before trust.
validateRawTarEntriesruns on outer and inner tars before path-keyed maps (duplicates, aliases, non-regular entries, bad paths).parseFacetArchiveenforces exactly two outer members and versionedparseBuildManifestDocument.Engine/CLI wiring. Registry download returns
DownloadedArchiveInfoand surfacesUNSUPPORTED_ARCHIVE; cachecachePutVerifiedtakes{ integrity, fileHashes };computeDirIntegrityhashes raw bytes for binary supplementary files. Publish/install render structured verification failures and basic upgrade hints.Release/docs. Adds the protocol changeset and refreshes OpenSpec migration/tasks for staged protocol → registry → adapter → held CLI rollout. Immutable
valid-0.1/valid-0.2fixtures anchor compatibility tests.Reviewed by Cursor Bugbot for commit c4e7771. Bugbot is set up for automated code reviews on this repo. Configure here.