golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 3 Packages Projects Releases Wiki Activity

fix(#2757): attribute transform-driven emitted changes and reclassify agents-verbatim (#2760)

Browse Source 
* fix(#2757): let derived provenance rules attribute a transform change

The Phase 2 provenance table (#2722) could only explain a moved emitted
path via its `sources`, so a `kind: 'derived'` artifact whose TRANSFORM
code changed (not its source) was unattributable by construction: 16
emitted agents/*.toml moved by PR #2566's runtime-artifact-conversion.cts
change, with zero agents/*.md in the diff.

Adds an optional per-rule `transforms: string[]`. A moved emitted path
now attributes via its source OR its transform, reusing sourceSatisfiedBy
so exact/prefix matching stays identical for both. Declared narrowly on
agents-toml-derived and agents-verbatim (the two families verified to
route through the same conversion pipeline) as
src/runtime-artifact-conversion.cts, src/install-effort-resolver.cts, and
src/model-catalog.cts — never bin/install.js, which would be a blanket
escape hatch spanning every installer concern.

Also corrects agents-verbatim from kind:'identity' to kind:'derived':
measured against origin/next's own fixtures, the same agents/<name>.md
hashes differently per runtime (e.g. codex vs claude), which a true
verbatim copy cannot do. Verified empirically by installing every
runtime and diffing against the raw repo source — none reproduce it
byte-for-byte; every one rewrites frontmatter and the hardcoded
.claude/ self-reference, and claude additionally gets an effort: line
injected. A new assertNoIdentityTransforms invariant rejects an
identity rule that declares a non-empty transforms list.

Closes #2757

* docs(#2757): flag PR #2566's transform-role files for later verification

src/agent-tools-contract.cts (added by #2566, absent on next) and
src/agent-install-check.cts (modified +112/-1 by #2566, read-only today)
were reviewed for prospective inclusion in AGENT_TRANSFORM_SRCS.

Excluded for now: the nonexistent file would fail this fix's own
"every declared transform path exists" hygiene test, and the modified
file's future role cannot be verified without the unmerged PR's diff.
Documents the reasoning, the interim ack-file safety net, and the
verification method to apply once these files stabilize, so the next
PR touching them has a pre-scoped one-line fix rather than a silent gap.

Related to #2757
This commit is contained in:
Tom Boucher
2026-07-28 10:47:43 -04:00
committed by GitHub
parent 9cbb48afa1
commit 44707c2c5e
4 changed files with 395 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
tests/helpers/emitted-diff.cjs
View File

@@ -195,6 +195,10 @@ function diffEmitted({
continue;
}
// Sources are checked before transforms so `via` is deterministic when a moved
// path is explained by both at once — the SOURCE is the more specific, more
// legible story ("the agent file changed") and is what a reviewer expects to
// see first, not an accident of iteration order.
let via = null;
for (const source of attribution.sources) {
const hit = sourceSatisfiedBy(source, changedSet);
@@ -204,6 +208,16 @@ function diffEmitted({
// non-empty template) but it is a footgun for the next rule author.
if (hit !== null) { via = hit; break; }
}
// #2757: a `derived`/`code-derived` artifact's bytes can also move because the
// TRANSFORM code that generates them changed, not the source it derives from —
// `sources` alone cannot express that. Reuses `sourceSatisfiedBy` unchanged so
// exact/prefix semantics stay identical for both lists.
if (via === null) {
for (const transform of attribution.transforms) {
const hit = sourceSatisfiedBy(transform, changedSet);
if (hit !== null) { via = hit; break; }
}
}
if (via !== null) {
attributed.push({ ...record, via });
@@ -211,7 +225,11 @@ function diffEmitted({
usedAcks.add(rel);
acked.push({ ...record, reason: ackEntries.get(rel).reason });
} else {
unattributable.push({ ...record, expectedSources: attribution.sources });
unattributable.push({
...record,
expectedSources: attribution.sources,
expectedTransforms: attribution.transforms,
});
}
}
}
@@ -279,7 +297,18 @@ function formatReport(result, { sampleLimit = 20 } = {}) {
if (result.unattributable.length) {
const list = result.unattributable.slice(0, sampleLimit)
.map((u) => ` ${u.runtime}: ${u.rel}\n rule ${u.ruleId}; expected a change under ${u.expectedSources.join(' or ')}`);
.map((u) => {
// #2757: a rule may explain a moved path via its source OR its transform
// code; name whichever possibilities exist so the message tells the whole
// story, not just half of it.
const expected = [
u.expectedSources.length ? `a change under ${u.expectedSources.join(' or ')}` : null,
(u.expectedTransforms && u.expectedTransforms.length)
? `a transform change under ${u.expectedTransforms.join(' or ')}`
: null,
].filter(Boolean).join(', or ');
return ` ${u.runtime}: ${u.rel}\n rule ${u.ruleId}; expected ${expected}`;
});
parts.push(
`${result.unattributable.length} emitted path(s) changed that nothing in this diff explains:\n${list.join('\n')}` +
(result.unattributable.length > sampleLimit

129
tests/helpers/emitted-provenance.cjs
View File

@@ -83,6 +83,66 @@ const INSTALLER_SRC = 'bin/install.js';
/** Source file holding the Kimi root-agent literal (runtime-artifact-layout.cts:303). */
const KIMI_ROOT_AGENT_SRC = 'src/runtime-artifact-layout.cts';
/**
* Transform sources for the per-runtime agent-content pipeline (#2757).
*
* `runtime-artifact-conversion.cts` rewrites frontmatter (quoting, dropping `tools:`/
* `color:`), reformats the tools list, and rewrites the hardcoded `.claude/` self-
* reference to each runtime's own home (`applyAgentPathRewrites`,
* `normalizeAgentBodyForRuntime`, the `convertClaudeAgentTo*Agent` family).
* `install-effort-resolver.cts` (+ the `model-catalog.cts` primitives it calls)
* resolves the `reasoning_effort` value that both Claude's `.md` (`effort:` frontmatter,
* injected by `injectEffortFrontmatter` in bin/install.js) and Codex's `.toml`
* (`model_reasoning_effort`) embed — "the same config-driven precedence chain" per
* bin/install.js's own #443 comment.
*
* Verified empirically (#2757), not assumed: installing every runtime for a sample
* agent and diffing the output against the raw repo `.md` (after normalizing the
* install-time HOME/version substitutions the golden fixtures already normalize) shows
* NO runtime — including Claude — emits a byte-identical copy. Every one rewrites at
* least the frontmatter and the `.claude/skills` self-reference; Claude additionally
* gets `effort:` injected. This is why `agents-verbatim` below is `derived`, not
* `identity`, despite its (retained, historical) id.
*
* Deliberately excludes `bin/install.js`: that file also implements the final splice
* (`injectEffortFrontmatter`, `generateCodexAgentToml`), but at 13k+ lines spanning
* hooks, MCP config, uninstall, and every other installer concern, declaring it here
* would be the blanket escape hatch ADR-2719 warns against — almost any PR touches SOME
* line of it. A change localized to those two functions and not reachable through the
* three files below stays unattributable and falls to the drift-ack file, which is the
* documented escape hatch for exactly that case.
*
* ── Known follow-up, NOT included here on purpose (#2757 review) ────────────
* The issue text also named `src/agent-tools-contract.cts` and
* `src/agent-install-check.cts` (both touched by PR #2566, "derive Codex agent sandbox
* from the tool"). Verified against THIS tree and excluded on the evidence:
* - `src/agent-tools-contract.cts` does not exist on `next` — #2566 ADDS it (+119/-0).
* A nonexistent path here would immediately fail the
* "every declared transform path exists in the repo" hygiene test in
* emitted-provenance.test.cjs, which exists precisely to catch a rule (or a
* suggested transform, as here) that cites a file that doesn't back real content.
* - `src/agent-install-check.cts` exists today and is READ-ONLY (`getAgentsDir`,
* `checkAgentsInstalled` — no fs.writeFileSync, no content transform); #2566
* nearly doubles it (+112/-1). Whether the addition becomes a real content-writer,
* a larger read-only diagnostic surface, or a helper called BY an already-declared
* file cannot be determined without reading #2566's actual diff, which review is
* explicit should not be fetched here. Declaring it on a line-count guess risks
* the exact false-attribution failure mode this whole table exists to prevent — a
* rule that looks fixed while resolving to the wrong causal story.
* Interim safety net: if #2566 lands and either file becomes a real transform without
* this list being updated, the differential (Phase 3) will correctly flag the moved
* agent artifact as unattributable — that is the guard working, not a regression — and
* unblocks via `tests/emitted-drift-ack.json` until this list is verified and extended
* using the SAME method used for the three files above: build, install every runtime,
* diff the output against the raw repo source, confirm which file's absence/presence
* changes the bytes.
*/
const AGENT_TRANSFORM_SRCS = [
'src/runtime-artifact-conversion.cts',
'src/install-effort-resolver.cts',
'src/model-catalog.cts',
];
/**
* A `sources` entry ending in `/` is a PREFIX, not a file: it means "any repo path
* under this directory legitimately explains this emitted path". Used where an
@@ -139,6 +199,17 @@ function nativePluginDescriptor(runtime) {
// `roots` — emitted prefixes this rule applies under (null = match `rel` whole)
// `pattern` — matched against the root-stripped tail (or whole `rel` when roots is null)
// `sources` — (match, ctx) => string[] of repo-relative paths; [] only for `synthesized`
// `transforms` — OPTIONAL string[] of repo paths implementing the TRANSFORM that
// produces this rule's emitted bytes (#2757). A `derived`/`code-derived`
// artifact's bytes can move for a second reason `sources` alone cannot
// express: the transform code changed, not the source it derives from.
// Phase 3 (emitted-diff.cjs) attributes a moved path if the diff
// satisfies EITHER `sources` OR `transforms`, reusing the same
// `sourceSatisfiedBy` matcher for both so exact/prefix semantics stay
// identical. `kind: 'identity'` rules MUST NOT declare a non-empty
// `transforms` — enforced by `assertNoIdentityTransforms` below — because
// an identity copy's bytes can only move when its source moves; that is
// what makes it an identity.
//
// Rule ORDER CARRIES NO SEMANTICS. Exactly-one matching is enforced, so rules are
// mutually exclusive by construction and the table reads correctly in any order.
@@ -163,8 +234,21 @@ const PROVENANCE_RULES = [
sources: (m) => [`scripts/${m[0]}`],
},
{
// Historical id — retained even though, per #2757, this is no longer identity.
// Nothing else in the repo keys off this string (checked), and the `sources`
// shape below is unchanged, so renaming it would only widen the diff.
id: 'agents-verbatim',
kind: 'identity',
// #2757 (was `identity`): measured against origin/next's own committed fixtures,
// the SAME emitted agents/<name>.md hashes DIFFERENTLY per runtime (e.g. codex vs
// claude for gsd-nyquist-auditor.md) — impossible for a true verbatim copy.
// Verified empirically by installing every runtime and diffing the output against
// the raw repo source: no runtime reproduces it byte-for-byte. Every one rewrites
// frontmatter quoting and the hardcoded `.claude/` self-reference
// (src/runtime-artifact-conversion.cts); Claude additionally gets an `effort:`
// line injected (src/install-effort-resolver.cts + src/model-catalog.cts). See
// the #2757 design doc for the alternatives considered (per-runtime split,
// per-runtime `kind`) and why this wholesale reclassification was chosen instead.
kind: 'derived',
roots: ['agents'],
// Excludes `gsd.md`: that is Kimi's ROOT agent, built from a code literal and
// NOT a repo agent file. Without the exclusion it matched here and resolved to
@@ -176,6 +260,7 @@ const PROVENANCE_RULES = [
// resolved to a file that does not exist. Same false-attribution class.
pattern: /^(?!gsd\.md$)(?!.*\.agent\.md$)[^/]+\.md$/,
sources: (m) => [`agents/${m[0]}`],
transforms: AGENT_TRANSFORM_SRCS,
},
{
id: 'copilot-agent-rename',
@@ -194,6 +279,9 @@ const PROVENANCE_RULES = [
// from the same agents/<name>.md source.
pattern: /^([^/]+)\.toml$/,
sources: (m) => [`agents/${m[1]}.md`],
// #2757 (issue text, PR #2566): a change to the conversion/effort code can move
// every emitted .toml without touching any agents/*.md. See AGENT_TRANSFORM_SRCS.
transforms: AGENT_TRANSFORM_SRCS,
},
{
id: 'agents-subagent-derived',
@@ -414,7 +502,7 @@ function matchRules(rel, runtime, rules = PROVENANCE_RULES) {
/**
* Resolve the provenance of one emitted path.
* @throws when the path matches zero or more than one rule.
* @returns {{ruleId: string, kind: string, sources: string[]}}
* @returns {{ruleId: string, kind: string, sources: string[], transforms: string[]}}
*/
function attributeEmittedPath(rel, runtime, rules = PROVENANCE_RULES) {
const hits = matchRules(rel, runtime, rules);
@@ -435,9 +523,44 @@ function attributeEmittedPath(rel, runtime, rules = PROVENANCE_RULES) {
ruleId: rule.id,
kind: rule.kind,
sources: rule.sources(match, { rel, runtime }),
// #2757: always an array, never undefined, so callers (Phase 3's diffEmitted)
// never need a defensive `|| []`.
transforms: Array.isArray(rule.transforms) ? rule.transforms : [],
};
}
/**
* Invariant (#2757): a `kind: 'identity'` rule may not declare a non-empty
* `transforms`. An identity copy's bytes can only move when its source moves — that
* is what makes it an identity; a transforms list on an identity rule would silently
* readmit the exact false-attribution risk defect 2 found (a rule that is total and
* "passes" while resolving to the wrong causal story).
*
* Called once at module load against the real PROVENANCE_RULES (fails fast on a
* future authoring mistake) and exported so tests can drive it against an injected
* corrupted table, matching this module's existing injectable-table convention.
*
* @param {Array} rules rule table (injectable)
* @throws when any identity rule declares a non-empty transforms array
*/
function assertNoIdentityTransforms(rules = PROVENANCE_RULES) {
const violators = rules.filter(
(r) => r.kind === 'identity' && Array.isArray(r.transforms) && r.transforms.length > 0,
);
if (violators.length) {
throw new Error(
`emitted-provenance: identity rule(s) [${violators.map((r) => r.id).join(', ')}] ` +
'declare a non-empty "transforms" — an identity copy\'s bytes can only move when ' +
'its source moves, which is what makes it an identity. Reclassify the rule\'s ' +
'"kind" (e.g. to "derived") if it legitimately needs a transforms list.',
);
}
}
// Fail fast: a malformed table crashes at require time rather than passing silently
// until some test happens to exercise the corrupted rule.
assertNoIdentityTransforms(PROVENANCE_RULES);
// ─── Fixture loading ──────────────────────────────────────────────────────────
/**
@@ -549,6 +672,7 @@ module.exports = {
PROVENANCE_RULES,
SKILLS_ROOTS,
KIMI_ROOT_AGENT_SRC,
AGENT_TRANSFORM_SRCS,
SOURCE_PREFIX_SUFFIX,
HOOKS_ROOTS,
COMMANDS_SRC,
@@ -560,6 +684,7 @@ module.exports = {
assertSafeRelPath,
matchRules,
attributeEmittedPath,
assertNoIdentityTransforms,
loadManifests,
assertTotality,
};