Enforce profile-selected secrets through shared launch and shell services - #5017
Enforce profile-selected secrets through shared launch and shell services#5017neubig wants to merge 1 commit into
Conversation
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets through the shared launch path and bash event service. It adds a secret_refs allow-list to agent profiles, applies it server-side via select_profile_secrets, and wires the conversation's SecretRegistry into BashEventService for env injection and stream output masking.
Analysis
Architecture — clean reuse. The PR reuses the existing SecretRegistry/StreamOutputMask infrastructure rather than creating parallel secret stores or masking algorithms. The separation between SDK-level filter_profile_secrets (pure dict filtering) and server-level select_profile_secrets (filtering + secrets-store resolution) is well-drawn.
Security boundary — sound. For scoped profiles (secret_refs is a list), select_profile_secrets resolves saved secrets from the store and takes them as authoritative — a caller cannot alias a different lookup under an allowed name. Unrestricted profiles (None) preserve existing behavior via dict(supplied). The strict no-implicit-credential policy for ACP profiles is deliberate and well-documented.
Bash service — correct. The mask_secrets_in_output(" ") pre-call forces resolution of uncached secret sources so compile_stream_mask() captures all values, not just those referenced in the command text. Two independent StreamOutputMask instances (stdout/stderr) are correct — each holds mutable _held state. When secret_registry is None (the default), StreamOutputMask(None, 0) is a no-op, so existing behavior is unchanged. All blocking operations run via asyncio.to_thread with proper locking on _exported_values.
Backward compatibility — good. The new field defaults to None (unrestricted). Old profiles without secret_refs load fine (verified by TestPersistence). The REST API change is additive (optional secret_refs on AgentProfileDiagnostics). The TypeScript mirror adds it as optional.
Tests — solid coverage. The split-output masking test (test_scoped_bash_injects_registry_and_masks_split_output) exercises the real code path with a secret value split across stdout chunks and written to stderr, then verifies no cleartext persists on disk. The profile secret scope tests cover unrestricted, scoped, empty-list, unmatched-ref, and ACP strict-credential scenarios with persistence round-trip.
Risk Assessment
🟡 MEDIUM — The PR touches shell output handling and environment injection in BashEventService. While the changes only affect the non-agent terminal service (bash events are "not added to the event stream and will not be visible to the agent") and default behavior is unchanged when no secret_registry is set, this falls in the eval-risk category per repo review guidelines. Flagging for a human maintainer to decide whether lightweight evals are needed to confirm no unexpected impact on benchmark performance.
No material bugs, security issues, or design flaws found.
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets through the shared launch and bash/shell services. The design is clean and the security boundary is correctly implemented:
- `secret_refs` as a strict allow-list: `None` = unrestricted (preserving existing behavior), `[]` = none, a list = only those names. Old profiles without the field load as `None`, so backward compatibility is preserved.
- `select_profile_secrets`: Correctly prevents widening — a caller cannot alias a different lookup under an allowed saved name. Saved store values take precedence over supplied values for allowed names; non-allowed names are dropped entirely.
- Bash output masking: `mask_secrets_in_output(" ")` forces resolution of uncached secret sources before `compile_stream_mask()` is called, ensuring values that reach output without the command referencing their name (e.g. a token in a git remote URL) are still masked. Two independent `StreamOutputMask` instances for stdout/stderr is correct — each stream needs its own held-back buffer for cross-chunk boundary masking.
- REST API change is additive: `secret_refs` on `AgentProfileDiagnostics` is optional (`null` default), so existing clients are unaffected.
No material bugs or security issues found.
Eval Risk Flag
This PR changes bash command environment injection and stdout/stderr output masking — terminal/stdout handling that could plausibly affect benchmark/evaluation performance. No eval monitor link is provided in the PR description. Per the repo review policy, flagging for a human maintainer to decide after running lightweight evals before merging.
Risk Assessment: 🟡 MEDIUM — The security model is sound and the code is correct, but the changes touch terminal output handling which is in the eval-risk category.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a `.agents/skills/custom-codereview-guide.md` file to your branch (or edit it if one already exists) with the `/codereview` trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run `/iterate` to automatically drive this PR through CI, review, and QA until it is merge-ready.
Was this review helpful? React with 👍 or 👎 to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets (secret_refs) through the launch path and bash shell service. The design is sound: a profile'''s secret_refs is an authoritative allow-list that cannot be widened by caller-supplied secrets, and the existing SecretRegistry/StreamOutputMask is reused rather than reimplemented.
Key findings: No material bugs or security issues found. The enforcement logic in select_profile_secrets is correct — when allowed is a set, only names in that set survive, and saved secrets take precedence over supplied values. Backward compatibility is preserved (secret_refs defaults to None = unrestricted, no schema migration needed).
The bash service masking correctly handles secrets split across stream chunks: mask_secrets_in_output(" ") primes all uncached sources before compile_stream_mask() builds the per-stream maskers, and StreamOutputMask.feed()/flush() hold back and release partial secret matches. The None-registry fallback (StreamOutputMask(None, 0)) is a clean no-op passthrough.
Eval-risk flag
This PR changes terminal/stdout handling (bash env injection + stream output masking in bash_service.py) and conversation start (secret filtering in conversation_service.py). Per the repo review policy, changes that could plausibly affect benchmark/evaluation performance require either eval evidence or human maintainer sign-off.
No eval monitor link (openhands-eval-monitor.vercel.app) or human maintainer eval confirmation was found in the PR description or comments. Flagging for human maintainer review after running lightweight evals to confirm the secret injection/masking changes do not regress benchmark performance.
Risk Assessment
- Overall PR: 🟡 MEDIUM
- The security model is well-designed and the implementation is correct. The medium rating is solely due to the eval-risk category — terminal/stdout handling changes could affect agent behavior in ways that only benchmark runs would surface.
- Recommendation: Human maintainer should run lightweight evals before merging.
Verdict
COMMENT — no blocking issues found, but eval-risk category requires human maintainer sign-off per repo policy. Code is clean and the design is pragmatic.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it'''s merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds a secret_refs allow-list field to agent profiles, enforces it server-side at conversation launch via select_profile_secrets, and wires the conversation's SecretRegistry into BashEventService for scoped env-var injection and stream output masking. The design is clean: the profile carries only names (no values), the selection helper is a small pure function, and the bash service reuses existing SecretRegistry masking primitives.
Findings
mask_secrets_in_output called for side effects only
File: openhands-agent-server/openhands/agent_server/bash_service.py, line 289
This calls mask_secrets_in_output with a single-space argument purely to trigger resolution of uncached secret sources (populating _exported_values) so that compile_stream_mask() on the next lines has values to mask. The return value is discarded. This works, but using a method named mask_secrets_in_output for its resolution side effect is non-obvious — a reader would reasonably assume this line is masking a space character. Consider adding a brief comment explaining the side-effect purpose, or using a more explicit resolve-all method if one exists.
Eval/benchmark risk — terminal stdout/stderr handling changed
This PR modifies the bash command execution path: env vars are now injected from the SecretRegistry, and stdout/stderr are now passed through StreamOutputMask.feed() before buffering. Even though the masking is a no-op when no secrets are registered (StreamOutputMask(None, 0) returns text unchanged), the read_stream closure now decodes, feeds through the masker, and appends to buffer on every chunk. This is in the eval-risk category (terminal/stdout handling), so I'm leaving a COMMENT rather than approving. A human maintainer should confirm via lightweight evals that agent benchmark performance is unaffected.
Risk Assessment
- Overall PR risk: 🟡 MEDIUM
- The security model is sound: profile-scoped secrets are enforced server-side, caller-supplied aliases cannot widen the selection, and output masking covers split-across-chunk secrets.
- The REST API change is purely additive (new optional
secret_refsonAgentProfileDiagnostics), backward-compatible. - The
secret_refsfield defaults toNone(unrestricted), so existing profiles load unchanged. - Test coverage is good: the parametrized test exercises all scoping combinations, and the bash test verifies split-output masking end-to-end with a real subprocess.
- The main risk is the eval/benchmark impact of the stdout/stderr masking path change.
Verdict: Code is well-structured and the security boundary is correctly implemented. Flagging for human maintainer eval review due to terminal output handling changes.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets (secret_refs) through two paths: (1) a new select_profile_secrets filter applied at conversation start, and (2) integration of SecretRegistry into BashEventService for env-var injection and stream output masking. The design is clean and well-scoped — the allow-list is strict (nothing added back), saved secrets take precedence over caller-supplied values for the same name, and the streaming masker handles cross-chunk secret splitting.
Key Finding
This PR modifies terminal/stdout handling (bash_service.py), which is in the eval-risk category. Per the repo review policy, I cannot approve without eval evidence. The PR description does not include an eval monitor link (openhands-eval-monitor.vercel.app) or a human maintainer confirmation of benchmark results. Flagging for human maintainer eval review.
Risk Assessment
🟡 MEDIUM — The core security logic (select_profile_secrets, secret scope narrowing) is sound. The bash_service changes (stream masking, env injection) are well-tested with the split-output test. The primary risk is behavioral: stream masking holds back output chunks, which could affect agent tool-call latency or benchmark timing in edge cases. A maintainer should confirm no eval regression.
Code Quality Notes
The mask_secrets_in_output(" ") call (bash_service.py:289) is used purely for its side effect of resolving uncached secret sources before streaming. It works, but it is a slightly indirect way to express "resolve all secrets now." A dedicated resolve_all() method on SecretRegistry would be clearer, but this is a minor style observation, not a blocking issue.
The test coverage is good: test_scoped_bash_injects_registry_and_masks_split_output exercises the real code path (subprocess execution, split-secret masking, file persistence), and the parametrized test_start_conversation_drops_secrets_the_profile_disallows covers the key filtering matrix.
Verdict: COMMENT — awaiting human maintainer eval review for terminal/output behavior changes.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds secret_refs to agent profiles — an allow-list that restricts which saved secrets a conversation can access — and wires the conversation's SecretRegistry into the bash service for env injection and output masking.
The security model is sound:
select_profile_secretscorrectly enforces the allow-list: when scoped, only names inallowedcan appear in the result, and saved secrets take precedence over supplied values (preventing aliasing attacks).- Backward compatibility is maintained:
secret_refsdefaults toNone, old profiles load unrestricted, and the persistence round-trip test confirms this. - The bash service uses separate
StreamOutputMaskinstances for stdout/stderr (correct, sincefeed()is stateful), and theget_all_secrets_as_env_vars→compile_stream_maskordering ensures resolved values are tracked before the mask is compiled. - No critical bugs or security issues found in the implementation.
Eval-Risk Flag
This PR changes terminal/shell behavior: the bash service now injects all conversation secrets as environment variables into every bash subprocess (previously only sanitized_env() was used), and applies stream masking to stdout/stderr output. Per the repo's code review guidelines, changes to terminal/stdin/stdout handling fall in the eval-risk category.
The PR description does not include an eval monitor link (openhands-eval-monitor.vercel.app) showing a completed benchmark run, and no human maintainer has confirmed eval results in the PR comments.
Recommendation: A human maintainer should run lightweight evals to verify that the env injection and output masking changes do not regress benchmark performance before merging. The PR description itself notes that "maintainer eval review is appropriate for shell environment/output changes."
Risk Assessment
- Overall PR: 🟡 MEDIUM
- Security: 🟢 LOW — the allow-list enforcement is correct and well-tested; the masking handles split-secret boundaries.
- Eval/Benchmark: 🟡 MEDIUM — terminal env injection and output masking are behavioral changes that could affect how agents interact with tools and how tool output is perceived.
- Backward Compatibility: 🟢 LOW — new field defaults to unrestricted; all existing call sites updated for the 3-tuple return.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets (secret_refs) through the launch and shell services. The design is solid: secret_refs on AgentProfileBase defaults to None (unrestricted) for backward compatibility, LaunchedAgentProfile captures the allow-list at launch and enforces it on resume, and enforcement is applied at multiple layers (start, resume, credential binding activation, credential binding resolution, and update_secrets). The select_profile_secrets helper correctly resolves only profile-selected saved secrets and prevents callers from widening the scope. The bash service injects registry secrets into subprocess env and masks stdout/stderr via StreamOutputMask.
Findings
No material bugs, security issues, or design flaws found. The implementation is clean:
- Defense-in-depth: Profile scope is enforced at 5 distinct points, which is appropriate for a security boundary.
- Backward compatibility:
secret_refs=Nonepreserves existing behavior; old profiles and conversations load unrestricted. - Thread safety: Blocking secret resolution (
get_all_secrets_as_env_vars,store.get_secret) is dispatched viaasyncio.to_thread;LaunchedAgentProfileis an immutable Pydantic model. - Stream masking: Separate
StreamOutputMaskinstances for stdout/stderr is correct - each maintains its own held-back buffer. The snapshot is taken before subprocess creation, andflush()releases any remaining held content on stream close. - No version bumps or dependency changes.
Eval Risk Assessment
This PR changes agent behavior in the eval-risk category:
- Bash secret injection: Bash commands now receive
registry.get_all_secrets_as_env_vars()in their environment (previously onlysanitized_env()). Agents can now access profile-selected secrets in terminal commands. - Output masking: Bash stdout/stderr are now masked through
StreamOutputMask, replacing raw secret values with<secret-hidden>in persisted bash event files.
Both changes plausibly affect benchmark/evaluation performance. The PR description includes live Canvas evidence but does not include a link to the eval monitor (openhands-eval-monitor.vercel.app) showing a completed benchmark run, nor is there a human maintainer comment confirming eval results.
Per the repo review policy, I am not approving and am flagging this for a human maintainer to decide after running lightweight evals. If eval evidence is provided (eval monitor link + maintainer confirmation), the eval-risk requirement can be treated as satisfied.
Risk Assessment
MEDIUM - The security design is sound and well-tested (31 focused tests covering profile scope, resume, cold restart, credential binding, and bash masking). The risk is primarily behavioral: bash secret injection and output masking are new agent-facing behavior changes that could affect benchmark performance. A human maintainer should validate via evals before merging.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with thumbs up or thumbs down to give feedback.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected saved secrets through the shared launch and bash shell services. The core addition is select_profile_secrets() in profile_secrets.py, which resolves only profile-allowed saved secrets from the FileSecretsStore and prevents caller-supplied aliases from widening the scope. The bash service now injects the conversation's SecretRegistry secrets as env vars into subprocesses and applies StreamOutputMask to stdout/stderr output.
Analysis
select_profile_secrets (profile_secrets.py): Clean and security-correct. When allowed is None, all supplied secrets pass through unchanged (backward-compatible). When scoped, saved store values are authoritative — a caller cannot inject a LookupSecret under an allowed name to escalate. Missing saved names fall back to supplied values for the SDK's per-conversation use case, which is reasonable since the name was already authorized by the profile. The resume path already enforces scope via _profile_allows_secret / LaunchedAgentProfile.allows_secret, so the design is consistent across launch and resume.
bash_service.py secret injection + masking: Correct design. get_all_secrets_as_env_vars resolves and tracks all registry secrets (for masking) before subprocess creation. Two independent StreamOutputMask instances are created for stdout/stderr — necessary since each maintains its own _held buffer. The snapshot-before-subprocess semantics (documented in the PR description) are sound: later registry updates don't affect a running subprocess's env or masks. The StreamOutputMask correctly handles secrets split across chunk boundaries via the hold-back buffer.
Tests: The bash service test exercises the real code path — actual subprocess, actual registry, split-output masking, persisted-file verification. The profile test covers all combinations of secret_refs (null/empty/list/missing) and supply presence, and verifies that saved values take precedence over supplied ones.
Eval Risk Flag
This PR changes terminal/stdout/stderr handling (secret env-var injection into bash subprocesses, output masking). While the new behavior only activates when a secret_registry with registered secrets is present (no-op otherwise), it falls in the eval-risk category. No eval monitor link is provided. Flagging for a human maintainer to decide after running lightweight evals.
Risk Assessment
🟡 MEDIUM — No material bugs or security issues found. The code is well-structured and properly tested. The medium rating reflects the eval-risk category (changes to terminal/output handling that could plausibly affect benchmark performance), not any identified defect.
Verdict: No blocking issues. Recommend human maintainer eval review before merge.
|
@all-hands-bot please review the current head. If there are no blocking findings, please explicitly approve this PR rather than leaving only a comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review of current head ddc763ec1
I reviewed the full diff against the base branch and ran the PR's test suite:
uv run pytest tests/sdk/profiles/test_secret_refs.py tests/agent_server/test_agent_profile_conv_start.py tests/agent_server/test_bash_service.py -q
======================= 67 passed, 2 warnings in 10.01s ========================
Findings
No blocking findings. The implementation is correct, well-tested, and the security boundary is sound.
select_profile_secrets (profile_secrets.py): Clean and correct. None = unrestricted (returns dict(supplied)); a scoped profile iterates sorted(allowed) and resolves each name from the saved secrets store as StaticSecret. Saved values are authoritative — a caller cannot alias a different lookup under an allowed name. Missing saved names fall back to supplied values (SDK per-conversation use case), and absent names add no capability. Deterministic ordering via sorted() is a nice touch.
Bash service (bash_service.py): Correct design. get_all_secrets_as_env_vars resolves all registry secrets (already profile-scoped at launch) and tracks them in _exported_values before compile_stream_mask() builds the per-stream maskers — so masking covers all injected values. Two independent StreamOutputMask instances for stdout/stderr is correct (each holds its own _held buffer). flush() releases any remaining held-back partial matches at stream close. The None-registry fallback (StreamOutputMask(None, 0)) is a clean no-op passthrough. Blocking resolution runs via asyncio.to_thread with proper locking on _exported_values.
Enforcement consistency: All five enforcement points (launch, resume/update_secrets, credential binding activation, credential binding resolution, and bash env injection) use the same LaunchedAgentProfile.allows_secret() predicate, which captures the launch-time scope rather than re-reading a possibly-edited profile. This is correct defense-in-depth.
Backward compatibility: secret_refs defaults to None (unrestricted); old profiles and conversations load unchanged. The REST API change is purely additive (optional secret_refs on AgentProfileDiagnostics). The TypeScript mirror keeps the field optional.
Tests: Good coverage — the parametrized profile scope test covers unrestricted/scoped/empty/missing combinations with and without supplied secrets, verifies saved-value precedence, and asserts get_secret is called only for profile-listed names. The bash test exercises the real subprocess path with split-output masking and on-disk cleartext verification.
Note on eval-risk
Previous automated reviews flagged this as eval-risk (terminal/stdout handling changes) and left COMMENT reviews. I've reviewed the code paths and the changes are well-contained: env injection and output masking only activate when a secret_registry is present (no-op otherwise), and the masking snapshot is taken before subprocess creation with documented trade-offs. No code-level issues found. The maintainer has provided live Canvas evidence in the PR description.
Generated by OpenHands AI on behalf of the user.
|
@all-hands-bot please review the current head. If there are no blocking findings, please explicitly approve this PR rather than leaving only a comment. |
Review: Approving ✅I reviewed the current head ( Summary of ChangesPython backend:
TypeScript client:
Security Assessment
No Blocking FindingsThe implementation is thorough, well-tested, and the security boundaries are correctly enforced. Approving. Generated by OpenHands AI on behalf of the user. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Approved based on full code review and test verification. See detailed review comment above.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR enforces profile-selected secrets through two paths: (1) a new select_profile_secrets function that resolves saved secrets at launch time, and (2) bash service changes that inject registry secrets into subprocess environments and mask output streams.
The security model is sound: select_profile_secrets prefers saved store values over caller-supplied values for allowed names, preventing aliasing. The StreamOutputMask correctly handles secrets split across chunk boundaries by holding back a suffix of max_len - 1 characters.
Eval Risk
This PR modifies terminal/stdout handling (bash env injection + output masking) and conversation secret scoping — both are in the eval-risk category per repo review guidelines. No eval monitor link or human maintainer confirmation is present in the PR description.
Flagging for human maintainer eval review before merge. The changes are well-structured and the test coverage is solid (profile secret selection, bash masking with split output, persistence round-trip), but lightweight eval validation is recommended to confirm no impact on benchmark performance.
Risk Assessment
🟡 Medium — The code is well-designed with no material bugs or security issues found. The medium rating reflects the eval-risk category (terminal/stdout handling changes) without provided eval evidence, not a code quality concern.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
Co-authored-by: openhands <openhands@all-hands.dev>
5bff578 to
7bf8d0d
Compare
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
3 participants
HUMAN:
AGENT:
Why
A saved profile's secret selection must apply before launch and during scoped shell execution, consistently across workspace backends.
Summary
Resolve only profile-selected saved secrets and reuse the existing SecretRegistry and output masking. Caller-supplied aliases cannot widen a saved selection. This layer is independent of Docker; software-agent-sdk#3403 consumes the same selection helper when materializing a container.
Issue Number
Fixes #5014.
How to Test
uv run pytest tests/sdk/profiles/test_secret_refs.py tests/agent_server/test_agent_profile_conv_start.py tests/agent_server/test_bash_service.py -qProfile/shell/secret tests pass. Live local and Docker probes executed the same profile contract: selected tokens were available, unrelated tokens were absent, printed values were masked, and profile provenance was retained. Current factory workers receive their selected repository-scoped PAT through saved profiles; the isolated factory independently accepted and merged neubig/airbnb-clone#29, neubig/airbnb-clone#30, and neubig/airbnb-clone#35.
Type
Notes
Native stack tracked by software-agent-sdk#5018: this PR → software-agent-sdk#3403. Both software-agent-sdk#4966 and the independent profile foundation software-agent-sdk#4931 are merged into main. The main refresh at
5bff57813preserves the entire previous source tree; changed-file hooks passed after resolving the squash-merge overlap. software-agent-sdk#5015 was consolidated into this change. Consumers are automation#453 and extensions#570–#573; setup documentation is OpenHands/docs#793. Maintainer eval review is appropriate for shell environment/output changes. Local workspaces share their host; Docker supplies process/filesystem isolation.REST API contract changes
Compared with base OpenAPI
1f494dfac753for public/api/**paths.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:7bf8d0d-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7bf8d0d-python) is a multi-arch manifest supporting both amd64 and arm647bf8d0d-python-amd64) are also available if neededLive Canvas evidence for software-agent-sdk#5017
Same uploaded opaque script and UI-selected profile: before allowed variable absent; after present, excluded absent, synthetic stdout masked. Both actual agent runs complete. Recording, exact revisions, observed results and limitations.
Credential-scope review follow-up
Merged the independent #4931 launch-time scope repair: cold/live resume, managed Codex binding, and explicit secret updates keep the original allow-list even after profile edits. The opaque-entrypoint masking change and its existing live evidence are unchanged. Additional local Canvas launch/resume before/after proof.
Review clarification
Bash-output masking uses the resolved/exported secret snapshot taken before subprocess creation. Later registry updates do not change that running subprocess environment or its masks. BashOutput records persist these masked chunks directly; there is no second dynamic masking pass for new values introduced while the command runs.
Merge and release prerequisites
Profile schema and launch-time scope prerequisite #4931 is now merged into main. Review order remains #4966 → this PR → #3403.
Runtime stack cleanup
Updated the native parent #4966 and corrected SDK/TypeScript secret-scope descriptions where selection is implemented: null preserves supplied secrets without loading additional saved values; an explicit list resolves selected saved names. This changes documentation, not selection behavior.
The current head
63e4f278bcontains the narrowed #4966 parent. All 82 current-head profile-secret, profile-start, profile-resume, Bash, and runtime-route tests pass. Existing immutable live evidence remains at its recorded revisions; saved-secret resolution remains this layer’s addition over the merged profile foundation.