fix(cli): read secret values from stdin - #85
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesCLI 비밀 입력 보안
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant getpass
participant stdin as sys.stdin
participant SecretStore
alt TTY 입력
CLI->>getpass: 비에코 비밀값 요청
getpass-->>CLI: 비밀값 반환
else 비대화형 입력
CLI->>stdin: 제한된 한 논리 행 읽기
stdin-->>CLI: 비밀값 반환
end
CLI->>CLI: 입력값 검증
CLI->>SecretStore: 검증된 비밀값 저장
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/doctoring/cli-secret-input.md`:
- Line 29: Update the security statement in the CLI secret input documentation
to limit the claim to captured test output and test logs, or remove “tests” from
the claim; do not imply that test source code or assertions never contain the
supplied plaintext.
- Around line 21-22: Update the interactive terminal behavior in the CLI
secret-input documentation to specify that if getpass.getpass() cannot disable
terminal echo, input must not fall back to visible plaintext entry and the
command must terminate with ConfigError. Preserve the existing no-echo behavior
for successful echo control and document the fail-closed contract covered by
test_interactive_secret_refuses_getpass_echo_fallback.
- Around line 45-53: Update the CI acceptance evidence in the
development-sequence documentation to include the successful CI workflow run for
source head 44ffbe5298022652c497b3e1f0d28790839135e1, including its run ID and
verification of Python 3.10, 3.12, and 3.14 tests. Keep the existing RED run as
fail-first history, and do not count security-scan or review-workflow successes
as test acceptance evidence.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42dcec3d-21c0-4c0e-872c-00848f15bce8
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mddocs/doctoring/cli-secret-input.mdpg_llm_batch/cli.pytests/test_bootstrap_cli.pytests/test_cli_secret_input_security.py
|
@coderabbitai review Please review the exact current source head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="bf2cc2e140dc3ff4a56c3203f80f41bb9fed5d10"
head="e68d6453b0d50d27e091f82fae5c8d6365611101"
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}"
echo '=== exact changed files ==='
git diff --name-status "$base" "$head"
echo '=== exact implementation diff ==='
git diff --unified=80 "$base" "$head" -- pg_llm_batch/cli.py
echo '=== outlines ==='
ast-grep outline pg_llm_batch/cli.py --items all --view expanded
ast-grep outline tests/test_cli_secret_input_security.py --items all --view expanded
echo '=== focused test diff ==='
git diff --unified=40 "$base" "$head" -- tests/test_cli_secret_input_security.py tests/test_bootstrap_cli.py
echo '=== secret-related definitions and error paths ==='
rg -n -C 5 -P '(^\s*(def|class)\s+.*(secret|Secret|config|Config)|\b(set-secret|SecretStore|ConfigError|getpass|GetPassWarning|stdin|stderr)\b)' \
pg_llm_batch/cli.py tests/test_cli_secret_input_security.py tests/test_bootstrap_cli.pyLength of output: 49561 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== exception types and formatting ==='
fd -t f -e py . pg_llm_batch tests | sort | xargs -r rg -n -C 5 -P \
'(^\s*class\s+\w*(Error|Exception)\b|def\s+(set_secret|error|main)\s*\(|raise\s+\w*(Error|Exception)\b|print\(.*stderr|str\([^)]*(exc|error))' \
|| true
echo '=== SecretStore implementation and callers ==='
fd -t f -e py . pg_llm_batch | sort | xargs -r rg -n -C 12 -P \
'(^\s*class\s+SecretStore\b|^\s*def\s+set_secret\s*\(|\.set_secret\s*\()' \
|| true
echo '=== parser construction and parser-error test cases ==='
fd -t f -e py . tests pg_llm_batch | sort | xargs -r rg -n -C 5 -P \
'(add_subparsers|add_parser|parse_args|parser\.error|unrecognized arguments|invalid choice|argument .*:|SystemExit)' \
|| true
echo '=== current documentation input and error contract ==='
git show e68d6453b0d50d27e091f82fae5c8d6365611101:docs/doctoring/cli-secret-input.md | nl -baLength of output: 98697 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== argparse nested-parser diagnostic reproduction (standard library only) ==='
python - <<'PY'
import argparse
import re
import sys
from contextlib import redirect_stderr
from io import StringIO
class RedactingParser(argparse.ArgumentParser):
def error(self, message):
message = re.sub(
r"(?s)^unrecognized arguments:.*$",
"unrecognized arguments: <redacted>",
message,
)
super().error(message)
parser = RedactingParser(prog="pg_llm_batch")
commands = parser.add_subparsers(dest="command", required=True)
config = commands.add_parser("config")
config_commands = config.add_subparsers(dest="config_command", required=True)
secret = config_commands.add_parser("set-secret")
secret.add_argument("secret_key")
for argv in (
["config", "set-secret", "key", "argv-secret"],
["config", "argv-secret"],
):
captured = StringIO()
try:
with redirect_stderr(captured):
parser.parse_args(argv)
except SystemExit:
pass
print(f"argv={argv!r}")
print(captured.getvalue())
PY
echo '=== exception hierarchy ==='
sed -n '1,115p' pg_llm_batch/exceptions.py
echo '=== full SecretStore write path ==='
sed -n '264,355p' pg_llm_batch/config.py
echo '=== CLI secret-input documentation ==='
sed -n '1,220p' docs/doctoring/cli-secret-input.md
echo '=== direct tests for main() error redaction and store failures ==='
rg -n -C 8 -P '(main\(|SecretStore|set_secret|stderr|redact|secret.*(error|fail)|error.*secret)' \
tests/test_cli_secret_input_security.py tests/test_bootstrap_cli.py tests/test_config.pyLength of output: 31024
I found no new actionable issues. The reviewed implementation removes the plaintext positional secret argument. It validates stdin input before I did not execute the full test suite in this sandbox.
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current heade68d6453b0d50d27e091f82fae5c8d6365611101. -
Head SHA:
e68d6453b0d50d27e091f82fae5c8d6365611101 -
Workflow run: 31302771937
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: cli-secret-input.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: cli-secret-input.md"]
R2 --> V2["docs review"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: cli-secret-input.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: cli-secret-input.md"]
R2 --> V2["docs review"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
|
|
@coderabbitai review |
|
@coderabbitai review Please review exact current source head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="bf2cc2e140dc3ff4a56c3203f80f41bb9fed5d10"
head="e68d6453b0d50d27e091f82fae5c8d6365611101"
git cat-file -e "${base}^{commit}"
git cat-file -e "${head}^{commit}"
echo '=== exact commit range ==='
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
echo '=== changed CLI implementation ==='
git diff --unified=100 "$base" "$head" -- pg_llm_batch/cli.py
echo '=== CLI structure ==='
ast-grep outline pg_llm_batch/cli.py --items all --view expanded
echo '=== exceptions, configuration, and SecretStore structure ==='
ast-grep outline pg_llm_batch/exceptions.py --items all --view expanded
ast-grep outline pg_llm_batch/config.py --items all --view expanded
echo '=== relevant CLI source slices ==='
rg -n -C 12 -P \
'(^\s*(def|class)\s+|ConfigError|SecretStore|getpass|GetPassWarning|stdin|stderr|set-secret|parse_args|ArgumentParser|unrecognized arguments|close\s*\(|__enter__|__exit__)' \
pg_llm_batch/cli.py pg_llm_batch/exceptions.py pg_llm_batch/config.py
echo '=== focused test diff and structure ==='
git diff --unified=80 "$base" "$head" -- tests/test_cli_secret_input_security.py tests/test_bootstrap_cli.py
ast-grep outline tests/test_cli_secret_input_security.py --items all --view expanded
rg -n -C 8 -P \
'(argv|unrecognized arguments|stdin|getpass|GetPassWarning|ConfigError|SecretStore|close\s*\(|stderr|secret)' \
tests/test_cli_secret_input_security.py tests/test_bootstrap_cli.py
echo '=== all project SecretStore lifecycle call sites ==='
fd -t f -e py . pg_llm_batch tests | sort | xargs -r rg -n -C 6 -P \
'(SecretStore\s*\(|with\s+SecretStore|\.close\s*\(|def\s+__enter__|def\s+__exit__)' || true
echo '=== current documentation contract ==='
sed -n '1,240p' docs/doctoring/cli-secret-input.mdLength of output: 138705
I found no new actionable issues in the requested scope.
The TTY path promotes The non-interactive path reads a bounded value. It removes only one terminal LF or CRLF. It rejects empty, multiline, carriage-return-containing, and oversized values before The changed I used new source evidence only. I did not execute project code or tests. You are interacting with an AI system. |
1 participant
Security boundary
config set-secretpreviously required plaintext provider credentials in process argv. This slice moves secret entry to bounded stdin/getpass, prevents parser reflection, and enforces a complete one-logical-line boundary while preserving the PostgreSQLSecretStore, Fernet bootstrap-key, provider, schema, and review-agent contracts.Behavior
secret_key;getpass; inability to disable echo fails closed before visible-input fallback;<redacted>beforeargparsestderr output;Test-first evidence
Earlier RED→GREEN cycles established stdin/argv behavior,
GetPassWarningfail-closed handling, rejected-argument confidentiality, and the complete logical-line separator boundary. CodeRabbit's three inline documentation findings are resolved; predecessor review/check evidence remains development provenance only.Current exact state
6418ae959b7a1188ba2bd5b888b862da5aceecdc.main:06f88be7f7c3a3ce5e756bc3bf03f298031a0286.4d474f941689bad613a03a48318663d845c81b51.31543539126, Security Scan31543539135, and SAST Semgrep31543539146are queued; queued evidence is not acceptance.CHANGES_REQUESTEDreview targets predecessore68d6453b0d50d27e091f82fae5c8d6365611101and reports a predecessor central coverage-evidence failure; it is not exact-head acceptance or a current-source finding. A current-head required OpenCode workflow must decide the current state.Dependencies and ownership
Repository-local exact-source governance #88 is now on protected main. Issue #90 CLI cancellation remains a separate ownership lane and must not race this branch or #87.
Merge boundary
Promote to Ready so required current-head semantic/central workflows can evaluate this exact source. Merge only if the unchanged head receives terminal success for every then-live repository/central CI, security, package/provenance and policy gate, remains mergeable against the then-live protected base, and has zero valid unresolved current findings. No predecessor, synthetic, queued, pending, cancelled, skipped, absent, status-only, author-only, infrastructure-only, rate-limited, or failed evidence transfers.
Detailed rationale and APA 7 references remain in
docs/doctoring/cli-secret-input.mdusing MITRE CWE-214 and Pythonargparse/getpassprimary documentation.