Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
feat: add archive command for batch evaluations and recommendations by padmak30 · Pull Request #1121 · aws/agentcore-cli · GitHub
Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat: add archive command for batch evaluations and recommendations by padmak30 · Pull Request #1121 · aws/agentcore-cli · GitHub
Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

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

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' feat: add archive command for batch evaluations and recommendations by padmak30 · Pull Request #1121 · aws/agentcore-cli · GitHub
Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat: add archive command for batch evaluations and recommendations by padmak30 · Pull Request #1121 · aws/agentcore-cli · GitHub
Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' feat: add archive command for batch evaluations and recommendations by padmak30 · Pull Request #1121 · aws/agentcore-cli · GitHub
Skip to content

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

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

feat: add archive command for batch evaluations and recommendations - #1121

Merged
padmak30 merged 1 commit into
previewfrom
feat/preview_archive
May 5, 2026
Merged

feat: add archive command for batch evaluations and recommendations #1121
padmak30 merged 1 commit into
previewfrom
feat/preview_archive

Conversation

@padmak30

Copy link
Copy Markdown
Contributor

Cherrypick 7586092

  • feat: add archive command for batch evaluations and recommendations. Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/ history file.

…1112)
* feat: add archive command for batch evaluations and recommendations.
Introduces a new top-level archive command with two subcommands: archive batch-evaluation and archive recommendation. Each calls the corresponding service delete API and removes the matching local .cli/
history file.
@padmak30
padmak30 requested a review from a teamMay 5, 2026 15:58
@github-actionsgithub-actionsBot added the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report

StatusCategoryPercentageCovered / Total
🔵Lines43.58%9957 / 22847
🔵Statements42.86%10578 / 24678
🔵Functions40.55%1680 / 4143
🔵Branches40.26%6426 / 15960
Generated in workflow #2398 for commit 2365b65 by the Vitest Coverage Report Action

@agentcore-cli-automation

Copy link
Copy Markdown

IAM policy changes go far beyond archive/delete and include a broken placeholder

docs/policies/iam-policy-user.json grows by 163 lines in this PR. Only the final BatchEvalAndRecommendation statement (with DeleteBatchEvaluation / DeleteRecommendation) is actually needed to support the new archive command. The rest adds a sweeping set of unrelated permissions to this preview branch:

  • AgentCoreResourceManagement (runtime, memory, evaluator, gateway, workload identity CRUD)
  • CloudFormationFullcloudformation:* on *
  • SsmParameterLookup, CloudFormationTemplateVerification
  • ImportTestIam, ImportTestPassRole, ImportTestS3 — appear to be bugbash-only
  • SecretsManager, CustomJwtCognitoSetup
  • HarnessManagement, HarnessPassRole
  • ConfigBundleManagement
  • HttpGatewayIamRoleManagement

Two concrete problems:

  1. Unreplaced placeholder. The ImportTestIam and ImportTestPassRole statements use "arn:aws:iam::ACCOUNT_ID:role/bugbash-agentcore-role" with a literal ACCOUNT_ID string. If a user applies this JSON directly, IAM will reject it as an invalid ARN. The other two policy files (iam-policy-boundary.json, iam-policy-cfn-execution.json) don't use this convention, so there's no documented substitution step either.
  2. Scope creep on a cherry-pick. The PR description says this is a cherry-pick for the archive command. The preview branch's iam-policy-user.json is currently 140 lines; this would balloon it to 303 lines with statements that have nothing to do with archive. Several of these (Harness, ConfigBundle, CloudFormation:*) look like they belong in separate, reviewed PRs.

Options:

  • (preferred) Drop every new statement except BatchEvalAndRecommendation from this PR, and open separate PRs for the other permission groups that actually need to land on preview.
  • Keep only the statements that reflect features already present on the preview branch, remove the ImportTest* bugbash entries entirely, and replace ACCOUNT_ID with * (or document a substitution step).

@agentcore-cli-automation

Copy link
Copy Markdown

requireProject() runs outside the try/catch and breaks the --json contract

In src/cli/commands/archive/command.tsx, executeArchive calls requireProject() on line 21, before the try block:

asyncfunctionexecuteArchive<Textends{status: string}>(cliOptions: {id: string;region?: string;json?: boolean},config: { ... }): Promise<void>{requireProject();// <-- renders Ink + process.exit(1) on failuretry{constregion=awaitgetRegion(cliOptions.region);
...

requireProject() (in src/cli/tui/guards/project.tsx) renders an Ink <FatalError/> and calls process.exit(1) when there's no project or when the cwd isn't the project root. It has no awareness of --json.

So if a user runs agentcore archive batch-evaluation --id foo --json from outside a project (or from a subdirectory), they get Ink-formatted TTY output on stdout instead of the { "success": false, "error": "..." } envelope every other branch of this command produces. Scripted consumers relying on --json will fail to parse it. The e2e tests don't catch this because they run from the project root.

Options:

  • Move the project check inside the try block, handle cliOptions.json explicitly, and emit a JSON error before exiting — mirroring the pattern already used in src/cli/commands/pause/command.tsx for the online-eval subcommand.
  • Or, since the service-side delete doesn't actually require a local project (only the local .cli/ cleanup does), drop requireProject() entirely and let deleteLocalBatchEvalRun / deleteLocalRecommendationRun throw when there's no project — that error is already caught and surfaced as localDeleteWarning, so the service delete still succeeds and the user still gets a clean JSON response.

@github-actionsgithub-actionsBot removed the agentcore-harness-reviewing AgentCore Harness review in progress label May 5, 2026
@padmak30

Copy link
Copy Markdown
ContributorAuthor

For the 2 comments from Harness review

  1. The additional IAM changes were added to fix a merge conflict, since both branches were out of sync for this file. The existing policy on main has ACCOUNT_ID, so keeping it for now. This policy is not used anywhere, only used for docs.
  2. All other existing commands (recommendations history, evals history) call requireProject() outside the try/catch — same pattern as archive. None of them handle the --json contract on a project-not-found failure; they all fall through to requireProject's Ink render + process.exit(1). So keeping the behavior consistent with other patterns and main.

Comment threaddocs/policies/iam-policy-user.json
@padmak30
padmak30 merged commit 0abe589 into previewMay 5, 2026
19 checks passed
@padmak30
padmak30 deleted the feat/preview_archive branch May 5, 2026 18:10
@agentcore-cli-automation

Copy link
Copy Markdown

Reviewed the diff. The two substantive issues I would have raised — the unrelated IAM policy additions / unreplaced ACCOUNT_ID placeholder in docs/policies/iam-policy-user.json, and requireProject() running outside the try/catch in executeArchive breaking the --json contract — are already flagged in the existing review comments, and the author has responded with their rationale. No additional serious issues to call out. LGTM.

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.

3 participants

@padmak30@agentcore-cli-automation@notgitika