Skip to content

Harden imported artifact remote page fallback #1231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lupuszr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Harden imported artifact remote page fallback #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/agent/src/imported-artifact.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,10 +665,12 @@ export class ImportedArtifactMethods extends DKGAgentBase {
if (page.denied || page.unavailable || page.hashMismatch || page.bytesB64 == null) {
return { response: page };
}
const pageByteLength = Buffer.byteLength(page.bytesB64, 'base64');
if (
page.offset !== requestedRange.offset ||
page.hash !== params.hash ||
(page.totalBytes != null && page.offset + Buffer.byteLength(page.bytesB64, 'base64') > page.totalBytes) ||
pageByteLength > requestedRange.maxBytes ||
(page.totalBytes != null && page.offset + pageByteLength > page.totalBytes) ||
(page.truncated && (page.nextOffset == null || page.nextOffset <= page.offset))
) {
return { response: { ...page, bytesB64: undefined, hashMismatch: true } };
Expand All @@ -695,6 +697,10 @@ export class ImportedArtifactMethods extends DKGAgentBase {
if (first.offset !== 0 || first.hash !== params.hash) {
return { response: { ...first, bytesB64: undefined, hashMismatch: true } };
}
const firstByteLength = Buffer.byteLength(first.bytesB64, 'base64');
if (firstByteLength > IMPORTED_ARTIFACT_MAX_PAGE_BYTES || first.offset + firstByteLength > total) {
return { response: { ...first, bytesB64: undefined, hashMismatch: true } };
}
const chunks = [Buffer.from(first.bytesB64, 'base64')];
let nextOffset = first.nextOffset;
while (first.truncated && nextOffset != null && nextOffset < total) {
Expand All @@ -707,10 +713,13 @@ export class ImportedArtifactMethods extends DKGAgentBase {
if (page.denied || page.unavailable || page.hashMismatch || page.bytesB64 == null) {
return { response: page };
}
const pageByteLength = Buffer.byteLength(page.bytesB64, 'base64');
if (
page.offset !== requestedOffset ||
page.hash !== params.hash ||
pageByteLength > IMPORTED_ARTIFACT_MAX_PAGE_BYTES ||
(page.totalBytes != null && page.totalBytes !== total) ||
page.offset + pageByteLength > total ||
(page.truncated && (page.nextOffset == null || page.nextOffset <= requestedOffset || page.nextOffset > total))
) {
return { response: { ...page, bytesB64: undefined, hashMismatch: true } };
Expand Down
37 changes: 37 additions & 0 deletions packages/agent/test/imported-artifact.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,43 @@ describe('generic imported artifact peer handler', () => {
}));
});

it('rejects an oversized unverified remote page', async () => {

@otReviewAgent otReviewAgent Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: This only exercises the cache: false readRequestedPage path. The PR also added decoded-length guards in the cache-promotion path for the first page and subsequent pages, which is the path that can assemble verifiedBytes and promote bytes into the file store. A regression in those guards would not fail this suite. Add cache: true cases for an oversized first page and an oversized later page, asserting hashMismatch, bytesB64 cleared, and no verifiedBytes.

const bytes = Buffer.from('abcdef');
const artifactHash = keccakHash(bytes);
const agent = {
readAssertionArtifact: vi.fn(async ({ offset = 0 }) => ({
version: 1,
contextGraphId,
assertionUri,
kind: 'source',
hash: artifactHash,
offset,
totalBytes: bytes.length + offset,
truncated: false,
bytesB64: bytes.toString('base64'),
})),
};

const res = await ImportedArtifactMethods.prototype.fetchAndVerifyAssertionArtifact.call(agent, {
contextGraphId,
assertionUri,
kind: 'source',
hash: artifactHash,
offset: 1,
maxBytes: 3,
sourcePeerId: 'peer-local',
cache: false,
});

expect(res.response.hashMismatch).toBe(true);
expect(res.response.bytesB64).toBeUndefined();
expect(res.verifiedBytes).toBeUndefined();
expect(agent.readAssertionArtifact).toHaveBeenCalledWith(expect.objectContaining({
offset: 1,
maxBytes: 3,
}));
});

it('serves requested pages for artifacts larger than the cache promotion cap', async () => {
const firstPage = Buffer.from('first page');
const requestedPage = Buffer.from('requested page');
Expand Down
9 changes: 8 additions & 1 deletion packages/cli/src/daemon/routes/knowledge-assets-import.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ async function fetchFirstAvailableAssertionArtifact(
resolved: AssertionArtifactResolution,
opts: {
sourcePeerIds: string[];
explicitSourcePeerId?: boolean;
offset: number;
maxBytes: number;
cache: boolean;
Expand All @@ -168,7 +169,12 @@ async function fetchFirstAvailableAssertionArtifact(
if (remote.verifiedBytes) return { availability: 'verified', remote, sourcePeerId };
const page = remote.response;
if (!page.denied && !page.unavailable && !page.hashMismatch && page.bytesB64 != null) {
return { availability: 'unverified_page', remote, sourcePeerId };
const result: AssertionArtifactRemoteResult = { availability: 'unverified_page', remote, sourcePeerId };
if (opts.cache && !opts.explicitSourcePeerId) {
fallback ??= result;

@otReviewAgent otReviewAgent Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: fallback ??= result leaves an earlier unavailable/denied/hash-mismatch fallback in place even after a later peer returns a usable unverified page. With default cache promotion and no explicit sourcePeerId, a candidate list like [unavailable-peer, large-artifact-peer] now scans past the large artifact's unverified page, but returns the first unavailable response when no fully verified peer is found. This regresses remote reads for large or otherwise non-cache-promotable artifacts. Replace lower-priority unavailable fallbacks with the first unverified_page fallback while still continuing to look for a verified peer.

This keeps the first fallback even when it is only unavailable, so a later valid unverified_page can be discarded. With discovered peers, cache enabled, and no explicit source, a sequence like peer-down then peer-large-valid-but-unverified now returns the earlier unavailable result instead of the usable unverified page. Prefer the unverified result over an unavailable fallback, or track fallback priorities separately.

continue;
}
return result;
}
fallback ??= { availability: 'unavailable', remote, sourcePeerId };
}
Expand Down Expand Up @@ -325,6 +331,7 @@ export async function handleKaImportArtifactRead(ctx: RequestContext): Promise<v
const cache = raw.cache !== false && offset === 0;
const fetched = await fetchFirstAvailableAssertionArtifact(agent, resolved, {
sourcePeerIds,
explicitSourcePeerId: Boolean(sourcePeerId),
offset,
maxBytes,
cache,
Expand Down
76 changes: 76 additions & 0 deletions packages/cli/test/import-artifact-routes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,82 @@ describe('import artifact daemon routes', () => {
await expect(fileStore.get(artifactHash)).resolves.toEqual(bytes);
});

it('keeps scanning discovered peers after an unverified page when cache promotion can verify a later peer', async () => {
const staleBytes = Buffer.from('# Stale Candidate\n');
const bytes = Buffer.from('# Verified Candidate\n');
const artifactHash = sha256Hash(bytes);
const contextGraphId = 'cg-public-open-discovered-unverified-fallback';
const assertionName = 'imported-md';
const assertionUri = contextGraphAssertionUri(contextGraphId, 'did:dkg:agent:source', assertionName);
const discoverAssertionArtifactCandidates = vi.fn(async () => ['peer-unverified', 'peer-good']);
const fetchAndVerifyAssertionArtifact = vi.fn(async (params: { sourcePeerId: string }) => {
if (params.sourcePeerId === 'peer-unverified') {
return {
response: {
version: 1,
contextGraphId,
assertionUri,
kind: 'markdown',
hash: artifactHash,
offset: 0,
totalBytes: staleBytes.length,
truncated: false,
contentType: 'text/markdown',
bytesB64: staleBytes.toString('base64'),
},
};
}
return {
response: {
version: 1,
contextGraphId,
assertionUri,
kind: 'markdown',
hash: artifactHash,
offset: 0,
totalBytes: bytes.length,
truncated: false,
contentType: 'text/markdown',
bytesB64: bytes.toString('base64'),
},
verifiedBytes: bytes,
};
});
const { agent } = makeAgent({
contextGraphId,
assertionName,
assertionUri,
fileHash: artifactHash,
markdownHash: artifactHash,
markdownForm: `urn:dkg:file:${artifactHash}`,
onChainPolicy: { accessPolicy: 0, publishPolicy: 1 },
discoverAssertionArtifactCandidates,
fetchAndVerifyAssertionArtifact,
});
await startRoutes({ agent });

const read = await post('/api/knowledge-assets/import-artifact/read', {
contextGraphId,
assertionUri,
kind: 'markdown',
hash: artifactHash,
maxBytes: 1024,
});

expect(read.status).toBe(200);
expect(read.body).toMatchObject({
status: 'fetched',
hash: artifactHash,
bytesB64: bytes.toString('base64'),
source: { peerId: 'peer-good' },
});
expect(fetchAndVerifyAssertionArtifact.mock.calls.map(([params]) => params.sourcePeerId)).toEqual([
'peer-unverified',
'peer-good',
]);
await expect(fileStore.get(artifactHash)).resolves.toEqual(bytes);
});

it('derives non-owner public + open read metadata from replicated SWM linkage when _meta is absent (#872 devnet)', async () => {
// Devnet peers receive the promoted SWM assertion triples but do not
// receive the origin node's CG-root `_meta` rows. The read route should
Expand Down
Loading