feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat: add JSONL draft diff logic and update command support - #1926

Merged
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli
Aug 13, 2026
Merged

feat: add JSONL draft diff logic and update command support#1926
nborges-aws merged 7 commits into
refactorfrom
datasets-update-cli

Conversation

@nborges-aws

@nborges-awsnborges-aws commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

Adds eval dataset update, which reconciles a local JSONL draft file into the remote dataset DRAFT. Update is append-only at the service level, so the CLI cannot replace the JSONL file wholesale. Instead, we have to compute a diff between the local draft and the remote draft, then apply the required dataset mutations:

  • add examples missing from the remote draft
  • update examples where exampleId exists remotely but has content changes
  • delete remote examples no longer present in local draft
  • ensure untouched examples remain intact

Summary of changes:

  • Adds eval dataset update --id <dataset-id> --file-path <path>.
  • Downloads the remote DRAFT through the presigned downloadUrl from GetDataset. I considered using the ListDatasetExamples API for this, but this would've required N API calls, where N is ceil(exampleCount / pageSize).
  • Adds dataset diff logic for classifying additions, updates, deletes, and unchanged rows.
  • Writes exampleId for newly added examples back into the local JSONL file, so future updates reconcile correctly.
  • Validates local JSONL to ensure valid JSON objects, exampleId as non-empty strings, and rejects duplicate exampleId
  • Validate remote exampleId's are unique and responses have one ID per example
  • Adds shared IO helpers for JSONL parsing and generic text file reads
  • Move dataset download plumbing into fetch method, to be reused between eval dataset get --file-path and update’s remote draft fetch
  • Adds polling logic to await dataset to return ACTIVE during update process. Since update is a series of batch mutations, this is necessary. Otherwise, updates would fail if status was "UPDATING/DELETING" etc. during an attempted update

Related Issue

Closes #

Documentation PR

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

Manually tested commands using bun run ... against personal account with following procedure:

  • Created dataset from local JSONL (and waited until ACTIVE)
  • downloaded remote draft
  • edited local JSONL with one add, one update, one delete
  • ran eval dataset udpate
  • confirmed returned counts: added: 1, updated: 1, deleted: 1, unchanged: 0
  • confirmed local JSONL rewritten with new exampleId
  • downloaded remote draft, confirmed it matched the updated local file
  • published the updated draft

Full suite against latest HEAD after rebasing onto refactor:

  • bun test (851 pass, 0 fail)
  • bun run typecheck clean
  • bun run lint:check clean
  • bun run format:check clean
  • bun run build clean

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Stack created with GitHub Stacks CLIGive Feedback 💬

@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.80237% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.95%. Comparing base (9d63f5b) to head (934b59a).
⚠️ Report is 1 commits behind head on refactor.

Files with missing linesPatch %Lines
src/core/datasetDiff.ts99.09%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## refactor #1926 +/- ##
============================================
+ Coverage 96.88% 96.95% +0.06% 
============================================
Files 342 346 +4 Lines 19263 19737 +474 ============================================
+ Hits 18663 19136 +473 - Misses 600 601 +1 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Aug 5, 2026
Base automatically changed from datasets-cli to refactorAugust 6, 2026 20:39

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add onProgress bar

Comment threadsrc/core/datasetDiff.ts Outdated
Comment on lines +37 to +38
`expected a non-empty string`,
{ meta: { line: lineNumber } },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know if lineNumber would be useful metadata for telemetry.

flag("id", "the ID of the dataset to update", z.string().optional()),
flag("file-path", "local JSONL file to reconcile into the DRAFT", z.string().optional()),
],
handle: async (ctx, flags) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

runDatasetExampleBatches runs batches serially and polls for ACTIVE after each one (up to 60s per batch). A large diff sits with no output for minutes, so the command looks hung. We already have the onProgress pattern in project/manager.tsx. Can we wire the batch loop into it and print "Applying update"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great call out! I hooked up the onProgress pattern, and it improves the experience alot while waiting on updates to complete. PR has been updated with changes

@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 9340e02 to 1214e97CompareAugust 7, 2026 15:32

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need golden tests for update flow.

return `${rows.map((r) => JSON.stringify(r)).join("\n")}\n`;
}

describe("parseJsonl", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need this tests? Will the handler tests cover this for us?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The handler test covers the CLI routing and flag handling. It doesn't directly exercise the diff calculation and rules, which is what these tests are for.

return { control: () => client, data: () => client, iam: () => client };
}

describe("EvalClient.updateDatasetExamples", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need these unit tests? Won't the handler tests cover this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Same as above for handler. These tests cover the delete/update/add mutations, batching, polling logic, etc. Different surface than handlers

jariy17
jariy17 previously approved these changes Aug 7, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice job. The update diff is pretty good!

},
"description": "Recorded fixture for the dataset commands",
"draftStatus": "MODIFIED",
"downloadUrl": "https://agentcoredatasets685197708687-284077270265-us-west-2-an.s3.us-west-2.amazonaws.com/685197708687/datasets/agentcore_cli_dataset_fixture-jzVpQaA5It/draft/dataset.jsonl?X-Amz-Security-Token=IQoJb3JpZ2luX2VjEJP%2F%2F%2F%2F%2F%2F%2F%2F%2F%2FwEaCXVzLXdlc3QtMiJIMEYCIQCflkdW2nGM6BMjGlmG2AV9i5D4WezB625lTpjdcrGZ6wIhAL6vK33WybFtnZ0UP%2BgXuKospslfqDD69y7%2Fzv0XXn%2BlKvIECFwQABoMMjg0MDc3MjcwMjY1IgzOAcYPrFp9BqEy2NQqzwQHK90X3idFVQP4puJ2qijfTwzzcj9xbawSUEAFYlrfw45YamqnalNNRxfK%2BeQmgA8Tj5QWWNn3noXXcQ8%2BFSwmqEkLElukt5IVBH59sUvJ13vjVUH%2Fsah%2FpCl9%2FzNs7O7rDnn0QrKQvGEHButw2ftwWobC9cijfwZWP1KXQc1hj8gLxpeNjQSH8RWAnGYsHL%2FwQVkg9AUr4Fc1sPFrdmH22Kyla62M5sJ0%2FBgB%2FsSSSqeG%2B%2FE7egRt5zCHDScu0%2FydT0GRzEkUc5TUZiB1wRi1lHRudPGfLuUGAr27Gr5RfhUvgwceu4AY6ShiBOhh8djgKD%2B7uqtgj41P4Hxg%2Ft%2FZY5PTYH9xATKM4CTyNZ6HO57xHeo2M%2BmeGIF3nLe7I53ruMq4onogm8srajiAhs1%2FPHxSSXx5ga4MwtB9pE%2BsM26JWD3QIVdu6T%2FAK5Y0CBvji2PP1jnr89nS%2B10FTYULZJn1DD7P%2F7R2idxvKDFzKY6z7tluDo3yybXSICtjapW0A72cg0vAzXrl6DKlZuZF6S9WEyKUcmDJeznwRqbSgrezL%2Bu2utpIcMdhfGsVaEvFxLuU5M9YkC7KA6rXJtn2zdm80olJN3EgOwIF%2Bf3FwBUxPCsd1F0JM4RTQC%2F4cuObwJ7hUHHZkodbAXjKPu%2B2hEGBtZZOJOB78O7liJ7xlRSaeMQl3ZETMcLKerIdlzKRRr%2BV%2F29yKHRx8TZI08Y4KoXaqrHmJi9ZaXwa8wwBwckMcrQ%2B6%2FWrWtitd6WCvzJeMwZ%2FaZEso59nRnxYtQww9sbY0wY6ogEFvsbxfePrmzBMVs4DXNp%2F3WCIpofAAPDT0lPsWqFcN8deD1qpDK4gOwprTsurb6NLaI1fjVnAFXEeYesR8WaPOvFHEIDVGh3U4uCWU9nDd4VRJFOaXGkunU5JMCfOBVCQpWU5bazXNdS3aJcr0MAhy62Ii%2BHJC5nm20JTPU%2BcEwKMxnfQVfl1b9Yr80zEWp52Y5G6DfFjb3XYuUu8HgDQCic%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20260807T190246Z&X-Amz-SignedHeaders=host&X-Amz-Credential=ASIAUEJCTET4RAA4PZHK%2F20260807%2Fus-west-2%2Fs3%2Faws4_request&X-Amz-Expires=300&X-Amz-Signature=ac646581e361f5b80fce8df15655aae1cc74921c8a7658272522788555d58634",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we sanitize presigned URLs before persisting fixtures? The changed GetDataset and golden fixtures contain the full X-Amz-Security-Token, credential scope, and signature. They are short-lived, but we still should not commit them publicly. fixtureFetch already keys by the stable pathname, so would it make sense to store a redacted copy while returning the real URL during recording, and redact the corresponding golden output?

Comment threadsrc/core/eval.tsx
try {
// The remote request has already succeeded, so checkpoint its IDs even
// if cancellation arrives before the next poll or batch.
await atomicWrite(filePath, nextLocalText);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was wondering if we need an optimistic check before replacing the local file here. The command may run for several minutes, but each checkpoint is rebuilt from the initial localExamples snapshot. I reproduced editing the JSONL while the Add request was running, and this write silently replaced that edit with the original row plus its assigned ID. Could we verify that the file still matches the last known contents before replacing it, and preserve the reconciled output separately if it changed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah adding this check makes total sense. I've updated the logic so we verify the expected state of the local file. If the file has changed during update we leave it untouched. Instead, we write the reconciled output to a separate recovery file, surface that files' path to the user, and stop before the next batch.

Comment threadsrc/core/eval.tsx
Comment on lines +601 to +607
// Build every batch before mutating the remote draft so an oversized
// individual example cannot fail after earlier phases have already run.
const deleteBatches = buildDatasetExampleBatches({
items: diff.deleteIds,
payloadItem: (exampleId) => exampleId,
requestBody: (exampleIds, clientToken) => ({ exampleIds, clientToken }),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the payload-size calculation include datasetId? These requestBody callbacks size only the examples and client token, while the actual SDK commands also include datasetId. I constructed a batch accepted as exactly 5 MB here whose actual command input was 115 bytes over the limit. Would it make sense to size the complete command input for all three mutation types so the preflight guarantee is accurate?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Great catch; absolutely should factor datasetId into the calc. Updated

Comment threadsrc/testing/fixtures.tsx Outdated
const path = fixturePath(dir, command);

if (isRecording()) {
const shouldWrite = !recordedPaths.has(path);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this first-response behavior break existing record/replay flows that poll the same request? For example, the Harness fixture records repeated GetHarness calls and relies on the final READY response being left in the fixture. With this global first-write rule, a fresh RECORD=1 run can preserve the initial CREATING response, and the next offline replay fails because the fixture is not settled. Would it make sense to support response sequences or scope this behavior to the Dataset update fixture?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good callout. I reset the last-response behavior in the shared recorder. Also moved the update fixture into its own directory, which prevents overwriting fixtures from the get fixture. This fixes the initial issue which made me switch to first reponse behavior originally

jariy17
jariy17 previously approved these changes Aug 13, 2026

@jariy17jariy17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aidandaly24
aidandaly24 previously approved these changes Aug 13, 2026
@nborges-aws
nborges-aws dismissed stale reviews from aidandaly24 and jariy17 via 3e08721August 13, 2026 16:27
@nborges-aws
nborges-awsforce-pushed the datasets-update-cli branch 2 times, most recently from 3e08721 to 4203c52CompareAugust 13, 2026 16:27
@nborges-aws
nborges-aws merged commit ca6426d into refactorAug 13, 2026
8 checks passed
@nborges-aws
nborges-aws deleted the datasets-update-cli branch August 13, 2026 16:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nborges-aws@codecov-commenter@aidandaly24@jariy17