DO NOT MERGE — retrospective Copilot review of merged PR #33 (e4ae226, vendor graph) - #38
DO NOT MERGE — retrospective Copilot review of merged PR #33 (e4ae226, vendor graph)#38mdheller wants to merge 1 commit into
Conversation
…#33) * engine 0.4.46: vendor graph — dependency staleness as a derived query prophet-platform ran engine 0.4.40 while main was 0.4.45. Five releases invisible in production, one of them a silent-wrong Cypher defect. A second, byte-identical copy in apps/lifecycle-warden went unnoticed the whole time, because nothing enumerated the copies. Merging an upstream PR does not ship it; only a re-vendor does. The fix is not a better spreadsheet. This makes freshness a DERIVED query: state the pins as typed nodes and edges, and recompute the verdict from the graph every time it is asked. ts/src/vendor-graph.ts implements the vfp: vocabulary from sociosphere PR #492 under its declared names rather than re-inventing them — vfp:Repository, vfp:Artifact, vfp:ConsumerApp, vfp:VendorPin, vfp:Release, vfp:Contract, and the edges vendors / producedBy / supersededBy / pinnedAt / pinFor / hostedIn / releasedAs / changesContract / guardedBy. Every class is KKO-typed in the AtomSpace TypeLattice: an Artifact is a kko:Products, and a VendorPin is a kko:RelationTypes because a pin is a REIFIED relation carrying its own policy, owner, disposition and dates — a governed object, not an edge label. Three derived questions, all pure (store.version() is identical before and after, asserted directly): 1. stalenessOf — the supersededBy path from the pin to the newest artifact: release distance AND the intervening artifacts, not a boolean. 2. blastRadiusOf — what breaks if I cut 0.4.46, answered BEFORE cutting it and without creating a node for it. Counted over ConsumerApp, never Repository: repo granularity is exactly what hid the lifecycle-warden copy. 3. contractCrossingRiskOf — whether the gap spans a release that moved a load-bearing contract, read from DECLARED changesContract edges. Never guessed from version numbers: 0.4.43 and 0.4.45 are both patch bumps and both moved contracts. The governed trigger emits an EffectRequest-shaped PROPOSAL against the sourceos-spec contract now vendored at ontology/effect-request/, sha-asserted at import exactly as SemanticAction is, with scripts/gen-effect-request.mjs mirroring gen-semantic-action.mjs. It is a proposal and nothing else — the engine never re-vendors, never opens a PR, never writes. Same rule the NLQ restricted search obeys. requiresHumanApproval is forced true whenever the gap crosses a contract; the membrane decides, outside this engine. The contract this plane speaks is itself a vendored copy subject to this plane. analyzeVendorFreshness seals like enrich/explore/nlq: sha256 over the derived output plus the {seq,nodes,edges} snapshot, so a staleness verdict is provable and replayable. requestedAt is taken from the caller rather than the clock — a wall-clock read would make the seal differ every run — and omitting it emits no proposals rather than inventing one. nlq.ts exports validateAgainst / assertSupportedKeywords / SchemaObj so both vendored contracts clear ONE validation subset guarded by ONE keyword bar, never two that can drift apart. format: date-time falls outside that subset, so vendor-graph checks requestedAt explicitly rather than leaving it unenforced. Measured against a fixture of the real situation: release distance 5, blast radius 2, crossesContract true over [receipt-shape, schema] at 0.4.43 and 0.4.45. Suite 389 -> 408, none broken. * review: close the CodeQL ReDoS in slug(), and make the contract check provable Two findings from review of #33. 1. CodeQL js/polynomial-redos (alert 35, the PR's one failing check) is a TRUE positive, not noise. `slug()` trimmed separators with `/^-+|-+$/`; on `x` + n dashes + `y` the `-+$` alternative is retried from every dash and each attempt scans to the end. Measured on the pre-fix code: 48 ms at n=10k, 2.8 s at n=80k, 11.4 s at n=160k, 146 s at n=400k — clean quadratic. `artifact_id` from a caller-supplied register becomes `pinKey`, which `slug()` puts straight into the EffectRequest `id`, and this is a published library, so the input is not ours to trust. Replaced with a linear index walk; same output. 2. `assert.deepEqual(p.contractViolations, [])` appeared in two tests, but an empty array is also what a validator that does nothing returns. The test named "validated against the sha-asserted vendored contract" only checked the pinned sha/specVersion/title — it proved the contract is PINNED, never that validation FIRES. Added a test that breaks a proposal against the same schema object across nine keywords (required, const, enum, type, pattern, minLength, uniqueItems, additionalProperties, nested properties) and requires a violation for each, so the green assertions above mean something. Both new tests were verified to fail against the defect they guard: the ReDoS test fails at 146 s with the old regex restored, and the teeth test fails on "required" with `validateAgainst` stubbed to a no-op. ts/src/nlq.ts is deliberately untouched — engine internals are out of lane.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a vendor-freshness “vendor graph” plane to model vendored artifacts as a typed graph and answer derived staleness/risk questions, including generating governed re-vendor EffectRequest proposals validated against a sha-pinned vendored schema.
Changes:
- Introduces
vendor-graph.tswith ingest + three pure graph queries (stalenessOf,blastRadiusOf,contractCrossingRiskOf) and sealed analysis (analyzeVendorFreshness). - Vendors and sha-pins the
EffectRequestJSON Schema, adds generator script, and validates proposals against the shared JSON-Schema subset validator. - Exports the new surfaces via
ts/src/index.ts, adds comprehensive tests, and bumps package version.
Reviewed changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/src/vendor-graph.ts | Implements vendor graph ingest, derived queries, proposal emission, and sealing |
| ts/src/vendor-graph.test.ts | Adds tests covering ingest, queries, purity, sealing, and schema validation “teeth” |
| ts/src/nlq.ts | Exports schema utilities so other vendored contracts can use the same validator + keyword bar |
| ts/src/index.ts | Re-exports vendor-graph and the embedded effect-request data for library consumers |
| ts/src/effect-request-data.ts | Generated embedded vendored schema bytes + pinned sha + specVersion |
| scripts/gen-effect-request.mjs | Generator to embed schema bytes and pinned sha in TS output |
| package.json | Bumps version to 0.4.46 |
| ontology/effect-request/PROVENANCE.md | Documents provenance and validation caveats of the vendored contract |
| ontology/effect-request/EffectRequest.json | Adds the vendored contract JSON Schema source file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface RevendorProposal { | ||
| pinId: string | ||
| /** Always `proposed`. An EffectDecision must precede any world change. */ | ||
| status: 'proposed' | ||
| effectRequest: EffectRequest | ||
| /** Violations of the vendored contract. Non-empty means the proposal is NOT emitted. */ | ||
| contractViolations: string[] | ||
| } |
| const proposals = opts.requestedAt === undefined ? [] : pins | ||
| .filter((p) => p.freshnessState === 'stale') | ||
| .map((p) => { | ||
| const repo = store.out(store.out(p.pinId, VFP_EDGE.pinnedAt)[0]?.id ?? '', VFP_EDGE.producedBy)[0] | ||
| const repoName = str(repo?.properties['repoName']) | ||
| const br = blastRadius.find((b) => b.repository === repoName) | ||
| return proposeRevendor(store, p.pinId, { | ||
| requestedAt: opts.requestedAt!, | ||
| ...(br ? { blastRadius: br.count } : {}), | ||
| }) | ||
| }) |
| function newestReleasedVersion(store: HellGraphStore, repository: string): string { | ||
| const repoNodeId = vfpId.repository(repository) | ||
| const produced = store.in(repoNodeId, VFP_EDGE.producedBy) | ||
| // The newest artifact is the one nothing supersedes. | ||
| const heads = produced.filter((a) => store.out(a.id, VFP_EDGE.supersededBy).length === 0) | ||
| const versions = heads.map((a) => str(a.properties['version']) ?? '').filter(Boolean).sort() | ||
| return versions[versions.length - 1] ?? 'unknown' | ||
| } |
| /** Walk `vfp:supersededBy` forward from `artifactNodeId`, longest path, cycle-safe. */ | ||
| function supersessionChain(store: HellGraphStore, artifactNodeId: string): string[] { | ||
| const chain: string[] = [] | ||
| const seen = new Set<string>([artifactNodeId]) | ||
| let cur = artifactNodeId | ||
| for (;;) { | ||
| // Deterministic when a release history forks: lowest node id wins, so the receipt is replayable. | ||
| const next = store.out(cur, VFP_EDGE.supersededBy) | ||
| .map((n) => n.id) | ||
| .filter((id) => !seen.has(id)) | ||
| .sort()[0] | ||
| if (next === undefined) return chain |
| const releaseId = vfpId.release(sourceId, version) | ||
| const rProps: Record<string, PropertyValue> = { version, sourceId } | ||
| put(rProps, 'observedAt', field<string>(r, 'observedAt')) | ||
| store.addNode(releaseId, [VFP.Release], rProps) | ||
| store.addEdge(VFP_EDGE.releasedAs, artifactId, releaseId) | ||
| stats.releases++ |
| test('the analysis seals like enrich/explore, and is deterministic', () => { | ||
| const a = analyzeVendorFreshness(fixture(), { requestedAt: '2026-07-29T00:00:00Z' }) | ||
| const b = analyzeVendorFreshness(fixture(), { requestedAt: '2026-07-29T00:00:00Z' }) |
Human adversarial pass — findings Copilot's review did not surfaceCopilot's review above independently confirmed two things found in a parallel manual pass Below are the findings the manual pass produced that Copilot did not report. All were verified 1. The sha-assertion does not guard the file its own provenance record names
Proven by flipping
Mutating a byte in the embedded The only independent anchor is the literal in PR #35 built exactly this gate for KKO four minutes later. There is no 2.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (5)
ts/src/vendor-graph.ts:1039
analyzeVendorFreshnessalready computespins(staleness) andrisks(contract crossings), butproposeRevendor()recomputesstalenessOf()+contractCrossingRiskOf()for every stale pin. On larger graphs this becomes redundant repeated traversals. Consider refactoring so proposals are built from the already-computedStalenessVerdict+ContractCrossingRisk(or expose an internal helper that accepts those precomputed values), keepingproposeRevendor()as a convenience wrapper.
const proposals = opts.requestedAt === undefined ? [] : pins
.filter((p) => p.freshnessState === 'stale')
.map((p) => {
const repo = store.out(store.out(p.pinId, VFP_EDGE.pinnedAt)[0]?.id ?? '', VFP_EDGE.producedBy)[0]
const repoName = str(repo?.properties['repoName'])
const br = blastRadius.find((b) => b.repository === repoName)
return proposeRevendor(store, p.pinId, {
requestedAt: opts.requestedAt!,
...(br ? { blastRadius: br.count } : {}),
})
})
ts/src/vendor-graph.ts:477
- This can create a
producedByedge tovfpId.repository('')(or to a repository node that was never created) when the matching source entry lacks a validrepo(sources with missingrepoare skipped in the earlier source ingest loop). Instead, readrepoand only add the edge when it is non-empty, ideally reusingensureRepo(repo, ...)so the target repository node is guaranteed to exist.
const src = sources.find((s) => field<string>(s, 'sourceId') === sourceId)
if (src) store.addEdge(VFP_EDGE.producedBy, pinnedArtifactId, vfpId.repository(field<string>(src, 'repo') ?? ''))
ts/src/vendor-graph.ts:165
- The exported policy name
'track-minor'is implemented using only the major component (majorOf) and the docstring describes major-based behavior. This mismatch will confuse API/manifest authors and makes the policy semantics ambiguous. Either rename the policy to reflect major-based tracking (e.g.,'track-major') or update the implementation to actually track minor (major.minor) as the name implies (and keep docs consistent).
export type FreshnessPolicy = 'pin-exact' | 'track-minor' | 'track-latest'
ts/src/vendor-graph.ts:591
- The exported policy name
'track-minor'is implemented using only the major component (majorOf) and the docstring describes major-based behavior. This mismatch will confuse API/manifest authors and makes the policy semantics ambiguous. Either rename the policy to reflect major-based tracking (e.g.,'track-major') or update the implementation to actually track minor (major.minor) as the name implies (and keep docs consistent).
const majorOf = (v: string): string => v.split('.')[0] ?? v
ts/src/vendor-graph.ts:644
- The exported policy name
'track-minor'is implemented using only the major component (majorOf) and the docstring describes major-based behavior. This mismatch will confuse API/manifest authors and makes the policy semantics ambiguous. Either rename the policy to reflect major-based tracking (e.g.,'track-major') or update the implementation to actually track minor (major.minor) as the name implies (and keep docs consistent).
const sameMajorNewer = intervening.some((a) => majorOf(a.version) === majorOf(pinnedVersion))
const majorMoved = releaseDistance > 0 && majorOf(latestVersion) !== majorOf(pinnedVersion)
freshnessState = sameMajorNewer || majorMoved ? 'stale' : 'current'
| export interface RevendorProposal { | ||
| pinId: string | ||
| /** Always `proposed`. An EffectDecision must precede any world change. */ | ||
| status: 'proposed' | ||
| effectRequest: EffectRequest | ||
| /** Violations of the vendored contract. Non-empty means the proposal is NOT emitted. */ | ||
| contractViolations: string[] | ||
| } |
| export function proposeRevendor(store: HellGraphStore, pinId: string, opts: ProposeOptions): RevendorProposal { | ||
| if (!ISO_DATE_TIME.test(opts.requestedAt)) { | ||
| // The shared subset validator implements `format: "uri"` only, so `requestedAt`'s | ||
| // `format: "date-time"` would otherwise go unchecked. Checked here rather than left unenforced. | ||
| throw new Error( | ||
| `vendor-graph: requestedAt '${opts.requestedAt}' is not an ISO-8601 date-time (the contract's ` + | ||
| 'format: date-time is outside the shared validator subset and is enforced here instead)') | ||
| } | ||
| const verdict = stalenessOf(store, pinId) | ||
| const risk = contractCrossingRiskOf(store, pinId) | ||
| const pin = store.getNode(pinId)! |
| const effectRequest: EffectRequest = { | ||
| id: `urn:srcos:effect:vendor-revendor.${slug(verdict.pinKey)}.${slug(verdict.pinnedVersion)}-${slug(toVersion)}`, | ||
| type: 'EffectRequest', | ||
| specVersion: EFFECT_REQUEST_SPEC_VERSION, | ||
| requestedByEventRef: opts.requestedByEventRef ?? `urn:srcos:vendor-freshness:${slug(verdict.pinKey)}`, | ||
| effectKind: 'update', | ||
| capability: 'vendor.revendor', | ||
| target: { | ||
| kind: 'vendor-pin', | ||
| identifier: verdict.pinKey, | ||
| ...(location !== undefined ? { location } : {}), | ||
| }, | ||
| parameters: { | ||
| fromVersion: verdict.pinnedVersion, | ||
| toVersion, | ||
| gapSize: verdict.releaseDistance, | ||
| blastRadius: opts.blastRadius ?? 0, | ||
| crossesContract: risk.crossesContract, | ||
| contractKinds: risk.contractKinds, | ||
| }, | ||
| // A re-emitted finding must not open a second PR. | ||
| idempotencyKey: `${verdict.pinKey}@${verdict.pinnedVersion}->${toVersion}`, | ||
| requiresHumanApproval: risk.crossesContract, | ||
| policyLabels: declaredLabels ? declaredLabels.split(',') : [], | ||
| riskLabels, | ||
| requestedAt: opts.requestedAt, | ||
| } | ||
|
|
||
| const contractViolations: string[] = [] | ||
| validateAgainst(EFFECT_REQUEST_SCHEMA as SchemaObj, effectRequest, 'EffectRequest', contractViolations) | ||
| return { pinId, status: 'proposed', effectRequest, contractViolations } | ||
| } |
| function newestReleasedVersion(store: HellGraphStore, repository: string): string { | ||
| const repoNodeId = vfpId.repository(repository) | ||
| const produced = store.in(repoNodeId, VFP_EDGE.producedBy) | ||
| // The newest artifact is the one nothing supersedes. | ||
| const heads = produced.filter((a) => store.out(a.id, VFP_EDGE.supersededBy).length === 0) | ||
| const versions = heads.map((a) => str(a.properties['version']) ?? '').filter(Boolean).sort() | ||
| return versions[versions.length - 1] ?? 'unknown' | ||
| } |
Evaluation — and this PR should be CLOSED, not mergedRetrospective review container for already-merged #33 (e4ae226, vendor graph). Its job was to get Copilot onto the vendor-graph reasoner after the fact; done, twice. It must not be merged — its base is Copilot coverage: 9 of 13 files, with the degraded-mode banner — so roughly a third of the diff was not looked at, and the findings below should not be read as exhaustive. Two rounds: 10 visible comments and 5 suppressed. Real — already owned by another lane
Confirmed alongside it: Another lane is fixing this in Real — not yet owned, and biting now
const versions = heads.map((a) => str(a.properties['version']) ?? '').filter(Boolean).sort()
return versions[versions.length - 1] ?? 'unknown'This is not a latent edge case — the engine is at 0.4.46/0.4.47 today: It feeds Real, minor
Rejected
DispositionNothing fixed here. The semver sort is the item worth raising as its own PR against |
Review complete — closing, not mergingThis PR did its job. It was never merge-eligible: it is byte-for-byte the already-merged content of #33 ( Recording the disposition of every finding here, because two of the three channels are easy to lose: Copilot left 10 inline comments and 5 more inside the Findings the review produced, and where they went
Items 7 and 8 are carried forward to the estate backlog by this comment. Item 8 should get a PR — either rename the policy to Branches
|
DO NOT MERGE — retrospective review only
This PR exists only to obtain a Copilot review of content that is already on
main.PR #33 ("engine 0.4.46: vendor graph — dependency staleness as a derived query") merged as
e4ae22682c6e5b231900c25d80d5b6cf9b03549fon 2026-07-30 without a Copilot review. This repo doesnot auto-request Copilot, and Copilot cannot review a closed PR.
The diff below is byte-for-byte PR #33: base is
dbe854f(mainimmediately before the merge),head is
e4ae226(the merge commit itself). Nothing here is new work.review-base/pr33→dbe854freview/pr33-retrospective→e4ae226main. Delete both once the review is captured.What already happened on #33
This is a missing second opinion, not a known defect.
Where a reviewer's attention is most useful
ts/src/vendor-graph.ts(1042 lines) — the whole surface, unreviewed by Copilot.EffectRequestschema (loadEffectRequestContract) — what itdoes and does not actually guard.
analyzeVendorFreshness→proposeRevendor:RevendorProposal.contractViolationsis computedand sealed but never branched on, while its own doc comment says "Non-empty means the proposal is
NOT emitted."
stalenessOf,blastRadiusOf,contractCrossingRiskOf) and thenew
export * from './vendor-graph'ints/src/index.ts.