chore: Cherry-picked changes from upstream - #40
Merged
Conversation
Collaborator
Author
|
🚀 PR Updated! The PR has been updated with the latest cherry-picked commits. @step-security/maintained-actions-dev Please review and approve the changes. 📦 Target Release Version: v1.0.167 |
ReviewClean, well-scoped change. Bounding the token-revocation One minor observation (non-blocking): LGTM. 👍 |
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code review summaryReviewed across quality, security, performance, tests, and docs. The ported source changes are high quality — the Two things worth the maintainer's attention before merge (beyond the inline comments): 1. The cherry-pick drops the fork's 2. |
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
amanstep
reviewed
Jul 10, 2026
Contributor
|
check why this files are missing |
amanstep
reviewed
Jul 10, 2026
amanstep
reviewed
Jul 10, 2026
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
Raj-StepSecurity
force-pushed
the
auto-cherry-pick
branch
from
July 10, 2026 09:44
b7ef4c8 to
b429c5a
Compare
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
Code review summaryReviewed via subagents (quality, security, tests, docs, performance). The security-sensitive work here is careful and generally fail-closed — nice work on the 1. No tests for 2. 3. Minor — trigger-time filter fails open on missing timestamps. 4. Minor doc — inline |
amanstep
previously approved these changes
Jul 10, 2026
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
| } | ||
|
|
||
| @retry( | ||
| retry=retry_if_exception_type((httpx.RequestError, httpx.HTTPStatusError)), |
There was a problem hiding this comment.
_graphql retries on any httpx.HTTPStatusError, including non-retryable 4xx (401/403), so an auth/permission failure burns all 3 attempts with exponential backoff before failing. The sibling _rest_request (line 1207) deliberately uses _retryable_http_error to retry only network errors and 5xx. These should share the same predicate.
Review summaryRan this through code-quality, performance, test-coverage, docs-accuracy, and security reviewers. Overall this is a large, high-quality cherry-pick — the new Biggest gap — the merge gate has zero tests. Verified-correct (calling out the good parts):
Minor:
|
amanstep
previously approved these changes
Jul 10, 2026
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
amanstep
approved these changes
Jul 10, 2026
🔍 Cherry-Pick Verification Report📦 Upstream Changes: 📋 File-by-File Analysis:
|
Review summaryReviewed across code-quality, security, performance, test-coverage, and docs. This is a well-executed cherry-pick — the security-sensitive changes hold up:
A few noteworthy items: 1. Working-note files are committed and should be dropped from the PR. Both 2. 3. Minor doc gap: the inline See the inline comment on |
| * tool can be granted to Claude without its MCP server being installed, or a | ||
| * server can be installed for a tool that was never granted (#1357). | ||
| * | ||
| * To stay in agreement it uses the same shell-quote tokenizer and the same |
There was a problem hiding this comment.
This docstring asserts the parse path "MUST stay in agreement" with parseClaudeArgsToExtraArgs and "uses the same shell-quote tokenizer" — but parse-sdk-options.ts now wraps its tokenize call in escapeShellMeta/unescapeShellMeta (to stop Bash(gh:*) collapsing to bare Bash), which this tokenize() does not mirror. It's harmless today because install decisions only key off mcp__* names (no shell metacharacters), but the stated invariant is now technically false, so a future tool name containing parens/pipes would be tokenized differently by the two paths. Worth either mirroring the escaping or softening this comment to note the intentional divergence.
Code Review SummaryI ran quality, security, performance, test-coverage, and docs passes over this cherry-pick. Overall this is high-quality, unusually well-documented code — the new One headline gap worth addressing before this gate is relied on: the 1828-line One documented sharp edge (not a code defect): native Approve reviews are not SHA-pinned the way Smaller items posted as inline comments (connect-timeout + block duplication in |
Code review summaryReviewed this cherry-pick across quality, security, performance, tests, and docs. Overall the security-critical logic is genuinely well-hardened, and I could not construct a working exploit against any of the areas this PR touches. A few things worth surfacing: Verified solid 👍
Noteworthy gaps
Low / optional
Nothing here is blocking. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.