fix(skills): decouple skill-manifest verify from local git tags (#10340)
* fix(skills): source released history from the committed ledger, not a tag walk verify:skill-bundle-manifest rebuilt the entire released-skill history by walking every local refs/tags/v* on each run and demanded byte-equality with the committed artifacts. Output was therefore a function of (skill bytes x local tag set x release timing), so any clone holding stray, deleted, or fork tags the committed artifacts predate rebuilt a divergent registry and failed lint. This was the 4th instance of one failure class (#8637 -> #9119 version bumps -> #9778 new tags -> local tag drift), each patched with a new tolerance rather than removing the tag coupling. Fix: the committed snapshot-registry + release-mapping ARE the released history; trust them instead of re-deriving from tags. - releasedHistoryFromCommitted() seeds generation from the committed ledger, dropping the floating unreleased tail (entries beyond what the mapping names). verify and --write are now pure functions of working-tree bytes with zero tag access. The tag walk survives only behind --rebuild-from-tags (disaster recovery), off the everyday path. - --release <version> + appendReleaseRow() perform the O(1) append of one mapping row at release cut (dedupes vs the last row, strips the v-prefix) -- the single authoritative point where working-tree bytes become an immutable released revision. - release-cut.yml runs generate --release "$VERSION" before the release commit (Node built-ins only, no install needed); pr.yml drops fetch-depth: 0 from the lint job since verify no longer needs tag history. Recognition is unaffected: the runtime uses knownSnapshots = registry.skills (all entries, incl. the tail committed at PR-merge time), so a missing mapping row only loses a version label, never recognition or the update nudge. Trade-off: lint no longer cross-checks committed historical snapshots against tags. A hand-edit to an old released entry is still caught by the runtime manifest<->registry consistency check when the current manifest points at it, and can be audited anytime with --rebuild-from-tags. Verified: verify passes committed-sourced; --write is zero-diff (byte parity); a planted stray v-tag no longer changes output; edit-stub -> --write -> --release appends the correct single row; double --release is idempotent; --rebuild-from-tags reproduces the committed artifacts. Generator tests 14 pass/ 1 skip; runtime skill-bundle-artifacts + freshness-inventory 14 pass; bundled skill guides verify passes. * fix(skills): keep one release-mapping row per version on a re-cut A cut that pushed the version bump to main but died before pushing the tag is re-cut at the same version. If skills changed in between, the second --release appended a duplicate row, and the stale one named revisions that tag never ships — which verify-skill-update-roundtrip then pairs with the tag's real bytes. Overwrite the trailing row instead (the tag is absent, so that version was never published). Refuse only when an earlier row claims the version, which the cut workflow already rejects upstream, so this cannot wedge a recovering cut.
This commit is contained in:
Brennan Benson
GitHub
parent
e058370c78
commit
8d61d76a59
|
|
@ -16,9 +16,9 @@ jobs:
|
|||
- name: Checkout
|
||||
uses: actions/checkout@v6
|
||||
with:
|
||||
# Why: the freshness registry is derived from immutable release tags,
|
||||
# so shallow PR checkouts cannot verify historical official identities.
|
||||
fetch-depth: 0
|
||||
# Why: verify:skill-bundle-manifest now checks working-tree bytes
|
||||
# against the committed skill ledger (advanced only at release cut) and
|
||||
# no longer walks release tags, so a shallow checkout is sufficient.
|
||||
persist-credentials: false
|
||||
|
||||
- name: Install native build tools
|
||||
|
|
|
|||
|
|
@ -521,7 +521,14 @@ jobs:
|
|||
# message and tag name explicitly (avoids npm's `v1.2.3` prefix
|
||||
# assumptions and any lifecycle scripts that would run on bump).
|
||||
npm version "$VERSION" --no-git-tag-version --allow-same-version
|
||||
git add package.json
|
||||
# Why: release cut is the single authoritative point where the released
|
||||
# skill ledger advances — this version's working-tree bytes become an
|
||||
# immutable released revision so future builds recognize them as an
|
||||
# official snapshot. It is the only place skill artifacts regenerate;
|
||||
# ordinary lint verifies working-tree bytes against the committed ledger
|
||||
# and never walks tags. Uses only Node built-ins, so no install needed.
|
||||
node config/scripts/generate-skill-bundle-manifest.mjs --release "$VERSION"
|
||||
git add package.json resources/skills
|
||||
commit_message="release: v$VERSION"
|
||||
if [[ "$EVENT_NAME" == "schedule" ]]; then
|
||||
commit_message="$commit_message [rc-slot:$SLOT]"
|
||||
|
|
|
|||
|
|
@ -370,14 +370,77 @@ function buildReleasedHistory() {
|
|||
return { registry, mapping }
|
||||
}
|
||||
|
||||
// Why: the artifacts must be pure functions of skills/ bytes and release-tag
|
||||
// history. Stamping the app version made every release cut invalidate the
|
||||
// committed output on all open branches and drag skill CI onto unrelated PRs.
|
||||
async function buildArtifacts() {
|
||||
// Why: released history is authoritative committed data, advanced only at
|
||||
// release cut. Seeding generation from the committed registry + mapping makes
|
||||
// ordinary verify/regeneration a pure function of working-tree bytes, so it
|
||||
// never walks git tags — the root cause of recurring lint drift when a clone
|
||||
// holds stray, deleted, or fork tags the committed artifacts predate.
|
||||
function releasedHistoryFromCommitted(committedRegistry, committedMapping) {
|
||||
const registry = { schemaVersion: SNAPSHOT_REGISTRY_SCHEMA_VERSION, skills: {} }
|
||||
const releasedSnapshotCounts = {}
|
||||
const mapping =
|
||||
committedMapping && committedMapping.schemaVersion === RELEASE_MAPPING_SCHEMA_VERSION
|
||||
? structuredClone(committedMapping)
|
||||
: { schemaVersion: RELEASE_MAPPING_SCHEMA_VERSION, releases: [] }
|
||||
if (committedRegistry && committedRegistry.schemaVersion === SNAPSHOT_REGISTRY_SCHEMA_VERSION) {
|
||||
const mappedCounts = releasedSnapshotCountsFromMapping(mapping)
|
||||
for (const [name, snapshots] of Object.entries(committedRegistry.skills ?? {})) {
|
||||
// The committed registry carries at most one unreleased tail beyond the
|
||||
// revisions named by the mapping; drop it and recompute it from bytes.
|
||||
const releasedCount = mappedCounts?.[name] ?? Math.max(0, snapshots.length - 1)
|
||||
registry.skills[name] = snapshots.slice(0, releasedCount)
|
||||
releasedSnapshotCounts[name] = releasedCount
|
||||
}
|
||||
}
|
||||
return { registry, mapping, releasedSnapshotCounts }
|
||||
}
|
||||
|
||||
// Why: disaster recovery only. Reconstruct released history from the immutable
|
||||
// release tags when the committed ledger must be rebuilt from scratch. Kept off
|
||||
// the verify/regenerate path — walking tags there is what coupled lint to the
|
||||
// executing clone's tag state and broke it on version bumps, new tags, and
|
||||
// stray/deleted local tags.
|
||||
function releasedHistoryFromTags() {
|
||||
const { registry, mapping } = buildReleasedHistory()
|
||||
const releasedSnapshotCounts = Object.fromEntries(
|
||||
Object.entries(registry.skills).map(([name, snapshots]) => [name, snapshots.length])
|
||||
)
|
||||
return { registry, mapping, releasedSnapshotCounts }
|
||||
}
|
||||
|
||||
// Why: release cut is the single authoritative point where working-tree bytes
|
||||
// become an immutable released revision. Append one mapping row for the version,
|
||||
// mirroring the historical dedupe where consecutive identical skill trees share
|
||||
// the earliest release's row.
|
||||
function appendReleaseRow(artifacts, version) {
|
||||
const appVersion = version.startsWith('v') ? version.slice(1) : version
|
||||
const currentRevisions = {}
|
||||
for (const skill of artifacts.currentManifest.skills) {
|
||||
currentRevisions[skill.name] = skill.releaseRevision
|
||||
}
|
||||
const releases = artifacts.releaseMapping.releases
|
||||
const last = releases.at(-1)
|
||||
if (last && isDeepStrictEqual(last.skills, currentRevisions)) {
|
||||
return
|
||||
}
|
||||
// Why: a cut that pushed the version bump to main but died before pushing the
|
||||
// tag is re-cut at the same version. If skills changed in between, appending
|
||||
// would leave two rows claiming this version and the stale one would name
|
||||
// revisions that tag never ships — overwrite, since the tag ships these bytes.
|
||||
if (last?.appVersion === appVersion) {
|
||||
releases[releases.length - 1] = { appVersion, skills: currentRevisions }
|
||||
return
|
||||
}
|
||||
// Why: an earlier row means re-cutting an already-shipped version, which the
|
||||
// cut workflow refuses upstream. Fail rather than corrupt shipped provenance.
|
||||
if (releases.some((release) => release.appVersion === appVersion)) {
|
||||
throw new Error(`Release mapping already has a row for ${appVersion}.`)
|
||||
}
|
||||
releases.push({ appVersion, skills: currentRevisions })
|
||||
}
|
||||
|
||||
async function buildArtifacts(releasedHistory) {
|
||||
const { registry, mapping, releasedSnapshotCounts } = releasedHistory
|
||||
const skillDirectories = (await readdir(SKILLS_ROOT, { withFileTypes: true }))
|
||||
.filter((entry) => entry.isDirectory())
|
||||
.map((entry) => entry.name)
|
||||
|
|
@ -557,13 +620,32 @@ async function verifyArtifacts(artifacts) {
|
|||
}
|
||||
|
||||
async function main() {
|
||||
const artifacts = await buildArtifacts()
|
||||
assertReleasedHistoryPreserved(
|
||||
await readCommittedRegistry(),
|
||||
artifacts,
|
||||
await readCommittedReleaseMapping()
|
||||
)
|
||||
await (process.argv.includes('--write') ? writeArtifacts : verifyArtifacts)(artifacts)
|
||||
const argv = process.argv.slice(2)
|
||||
const rebuildFromTags = argv.includes('--rebuild-from-tags')
|
||||
const releaseIndex = argv.indexOf('--release')
|
||||
const releaseVersion = releaseIndex >= 0 ? argv[releaseIndex + 1] : null
|
||||
if (releaseIndex >= 0 && !releaseVersion) {
|
||||
throw new Error('--release requires a version argument, e.g. --release 1.4.160')
|
||||
}
|
||||
|
||||
const committedRegistry = await readCommittedRegistry()
|
||||
const committedMapping = await readCommittedReleaseMapping()
|
||||
const releasedHistory = rebuildFromTags
|
||||
? releasedHistoryFromTags()
|
||||
: releasedHistoryFromCommitted(committedRegistry, committedMapping)
|
||||
const artifacts = await buildArtifacts(releasedHistory)
|
||||
|
||||
if (releaseVersion) {
|
||||
appendReleaseRow(artifacts, releaseVersion)
|
||||
}
|
||||
|
||||
// Why: released snapshots are append-only. The committed registry/mapping are
|
||||
// read fresh here so the artifacts (which may have appended a release row) can
|
||||
// never alias what we validate against.
|
||||
assertReleasedHistoryPreserved(committedRegistry, artifacts, committedMapping)
|
||||
|
||||
const shouldWrite = releaseVersion !== null || argv.includes('--write')
|
||||
await (shouldWrite ? writeArtifacts : verifyArtifacts)(artifacts)
|
||||
}
|
||||
|
||||
if (process.argv[1] && path.resolve(process.argv[1]) === import.meta.filename) {
|
||||
|
|
@ -574,6 +656,7 @@ if (process.argv[1] && path.resolve(process.argv[1]) === import.meta.filename) {
|
|||
}
|
||||
|
||||
export {
|
||||
appendReleaseRow,
|
||||
assertReleasedHistoryPreserved,
|
||||
buildArtifacts,
|
||||
buildReleasedHistory,
|
||||
|
|
@ -584,6 +667,7 @@ export {
|
|||
isToleratedReleaseMappingPrefix,
|
||||
normalizeText,
|
||||
packageDigest,
|
||||
releasedHistoryFromCommitted,
|
||||
sortManifestFiles,
|
||||
verifyArtifacts,
|
||||
writeArtifacts
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import { tmpdir } from 'node:os'
|
|||
import path from 'node:path'
|
||||
import { afterEach, describe, expect, it } from 'vitest'
|
||||
import {
|
||||
appendReleaseRow,
|
||||
assertReleasedHistoryPreserved,
|
||||
classifyFile,
|
||||
collectPackageFiles,
|
||||
|
|
@ -12,6 +13,7 @@ import {
|
|||
isToleratedReleaseMappingPrefix,
|
||||
normalizeText,
|
||||
packageDigest,
|
||||
releasedHistoryFromCommitted,
|
||||
sortManifestFiles
|
||||
} from './generate-skill-bundle-manifest.mjs'
|
||||
|
||||
|
|
@ -217,6 +219,102 @@ describe('skill bundle manifest generator', () => {
|
|||
expect(isToleratedReleaseMappingPrefix(serialized({ schemaVersion: 1 }), artifacts)).toBe(false)
|
||||
})
|
||||
|
||||
it('seeds released history from the committed ledger and drops the floating tail', () => {
|
||||
const snapshot = (releaseRevision, packageDigest) => ({ releaseRevision, packageDigest })
|
||||
const committedRegistry = {
|
||||
schemaVersion: 1,
|
||||
skills: {
|
||||
// released revs 1..2 named by the mapping, plus an unreleased tail at 3
|
||||
'orca-cli': [snapshot(1, 'aaa'), snapshot(2, 'bbb'), snapshot(3, 'unreleased')],
|
||||
// no mapping row -> fall back to all-but-tail
|
||||
'orca-linear': [snapshot(1, 'ccc'), snapshot(2, 'tail')]
|
||||
}
|
||||
}
|
||||
const committedMapping = {
|
||||
schemaVersion: 1,
|
||||
releases: [{ appVersion: '1.0.0', skills: { 'orca-cli': 2 } }]
|
||||
}
|
||||
|
||||
const seeded = releasedHistoryFromCommitted(committedRegistry, committedMapping)
|
||||
|
||||
// The unreleased tail is dropped; only mapping-named revisions survive.
|
||||
expect(seeded.registry.skills['orca-cli']).toEqual([snapshot(1, 'aaa'), snapshot(2, 'bbb')])
|
||||
expect(seeded.registry.skills['orca-linear']).toEqual([snapshot(1, 'ccc')])
|
||||
expect(seeded.releasedSnapshotCounts).toEqual({ 'orca-cli': 2, 'orca-linear': 1 })
|
||||
// The seed clones the mapping so a later release append cannot alias committed state.
|
||||
expect(seeded.mapping).toEqual(committedMapping)
|
||||
expect(seeded.mapping).not.toBe(committedMapping)
|
||||
})
|
||||
|
||||
it('returns an empty ledger when no committed artifacts exist', () => {
|
||||
const seeded = releasedHistoryFromCommitted(null, null)
|
||||
expect(seeded.registry.skills).toEqual({})
|
||||
expect(seeded.releasedSnapshotCounts).toEqual({})
|
||||
expect(seeded.mapping.releases).toEqual([])
|
||||
})
|
||||
|
||||
it('appends one release row, stripping the v-prefix and deduping identical tails', () => {
|
||||
const artifacts = {
|
||||
currentManifest: {
|
||||
skills: [
|
||||
{ name: 'orca-cli', releaseRevision: 36 },
|
||||
{ name: 'orca-linear', releaseRevision: 8 }
|
||||
]
|
||||
},
|
||||
releaseMapping: {
|
||||
schemaVersion: 1,
|
||||
releases: [{ appVersion: '1.4.151', skills: { 'orca-cli': 35, 'orca-linear': 8 } }]
|
||||
}
|
||||
}
|
||||
|
||||
appendReleaseRow(artifacts, 'v1.4.160')
|
||||
expect(artifacts.releaseMapping.releases.at(-1)).toEqual({
|
||||
appVersion: '1.4.160',
|
||||
skills: { 'orca-cli': 36, 'orca-linear': 8 }
|
||||
})
|
||||
|
||||
// A second release over identical revisions adds no row.
|
||||
appendReleaseRow(artifacts, '1.4.161')
|
||||
expect(artifacts.releaseMapping.releases).toHaveLength(2)
|
||||
})
|
||||
|
||||
it('overwrites the trailing row when a failed cut is re-cut at the same version', () => {
|
||||
const artifacts = {
|
||||
currentManifest: { skills: [{ name: 'orca-cli', releaseRevision: 37 }] },
|
||||
releaseMapping: {
|
||||
schemaVersion: 1,
|
||||
releases: [
|
||||
{ appVersion: '1.4.151', skills: { 'orca-cli': 35 } },
|
||||
// The failed cut already pushed this row to main at revision 36.
|
||||
{ appVersion: '1.4.160', skills: { 'orca-cli': 36 } }
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
appendReleaseRow(artifacts, '1.4.160')
|
||||
|
||||
// One row per version: the tag ships revision 37, so 36 must not linger.
|
||||
expect(artifacts.releaseMapping.releases).toEqual([
|
||||
{ appVersion: '1.4.151', skills: { 'orca-cli': 35 } },
|
||||
{ appVersion: '1.4.160', skills: { 'orca-cli': 37 } }
|
||||
])
|
||||
})
|
||||
|
||||
it('refuses to rewrite an already-shipped version behind the trailing row', () => {
|
||||
const artifacts = {
|
||||
currentManifest: { skills: [{ name: 'orca-cli', releaseRevision: 37 }] },
|
||||
releaseMapping: {
|
||||
schemaVersion: 1,
|
||||
releases: [
|
||||
{ appVersion: '1.4.151', skills: { 'orca-cli': 35 } },
|
||||
{ appVersion: '1.4.160', skills: { 'orca-cli': 36 } }
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
expect(() => appendReleaseRow(artifacts, '1.4.151')).toThrow(/already has a row for 1\.4\.151/)
|
||||
})
|
||||
|
||||
it.runIf(process.platform !== 'win32')(
|
||||
'rejects executable files in shipped skill packages',
|
||||
async () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue