Warn on placeholder GitHub token and expand act error extraction for auth failures (close #44)#47
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a sentinel-backed GitHub token resolver and warning emission, ensures those warnings bypass global quiet mode via a secondary console, expands executor error-keyword matching to include auth failures (401/403/rate-limit), integrates the resolver into CLI and command builder, and adds unit and integration tests. ChangesGitHub Token Resolution and Error Diagnostics
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as localci run
participant Resolver as resolve_github_token
participant CommandBuilder as ActCommandBuilder
participant WarnFn as warn_sentinel_github_token
participant ImportantOut as _important_console
User->>CLI: invoke run (--github-token? --offline?)
CLI->>Resolver: pass CLI token / env lookup
Resolver->>CLI: return resolved token
CLI->>CommandBuilder: populate secrets (GITHUB_TOKEN = resolved token)
CLI->>WarnFn: if not --offline, pass token
WarnFn->>ImportantOut: print_important_warning (if sentinel)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
cli/tests/test_executor.py (1)
612-626: ⚡ Quick winUse
tmp_pathfor the new executor log dirs.These new tests hardcode
/tmp/localci-test, which is POSIX-specific and can collide across parallel runs. Switching totmp_path / "localci-test"keeps them isolated and portable.Proposed fix
`@patch`("shutil.which") - def test_extract_error_http_401(self, mock_which): + def test_extract_error_http_401(self, mock_which, tmp_path): mock_which.return_value = "/usr/bin/act" - executor = JobExecutor(logs_dir=Path("/tmp/localci-test")) + executor = JobExecutor(logs_dir=tmp_path / "localci-test") output = "fetching action\nHTTP 401: Bad credentials\nend" extracted = executor._extract_error(output) assert "401" in extracted `@patch`("shutil.which") - def test_extract_error_http_403(self, mock_which): + def test_extract_error_http_403(self, mock_which, tmp_path): mock_which.return_value = "/usr/bin/act" - executor = JobExecutor(logs_dir=Path("/tmp/localci-test")) + executor = JobExecutor(logs_dir=tmp_path / "localci-test") output = "fetching action\nreceived HTTP status: 403\nend" extracted = executor._extract_error(output) assert "403" in extracted🤖 Prompt for 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. In `@cli/tests/test_executor.py` around lines 612 - 626, The tests use a hardcoded POSIX path for logs_dir; update both test_extract_error_http_401 and test_extract_error_http_403 to accept the pytest tmp_path fixture and construct the logs_dir from tmp_path (e.g. logs_dir=tmp_path / "localci-test") instead of Path("/tmp/localci-test") so they are portable and isolated; keep the existing JobExecutor(...) call but replace the Path(...) argument and add tmp_path as a function parameter to each test.
🤖 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 `@cli/localci/cli/run.py`:
- Around line 160-161: The sentinel warning is shown even in offline mode; only
call warn_sentinel_github_token when offline is false. After retrieving gh_token
via resolve_github_token(github_token), wrap the
warn_sentinel_github_token(gh_token) call in a conditional check (e.g., if not
offline:) so that warn_sentinel_github_token is skipped when the offline flag is
enabled.
---
Nitpick comments:
In `@cli/tests/test_executor.py`:
- Around line 612-626: The tests use a hardcoded POSIX path for logs_dir; update
both test_extract_error_http_401 and test_extract_error_http_403 to accept the
pytest tmp_path fixture and construct the logs_dir from tmp_path (e.g.
logs_dir=tmp_path / "localci-test") instead of Path("/tmp/localci-test") so they
are portable and isolated; keep the existing JobExecutor(...) call but replace
the Path(...) argument and add tmp_path as a function parameter to each test.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56a09f5c-84cd-48f5-9520-102fa06877cb
📒 Files selected for processing (7)
cli/localci/cli/run.pycli/localci/core/command_builder.pycli/localci/core/executor.pycli/localci/core/github_token.pycli/localci/utils/output.pycli/tests/test_executor.pycli/tests/test_github_token.py
|
@coderabbitai review this PR fully. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== github_token.py ==="
cat cli/localci/core/github_token.py
echo ""
echo "=== run.py ==="
cat cli/localci/cli/run.py
echo ""
echo "=== command_builder.py ==="
cat cli/localci/core/command_builder.py
echo ""
echo "=== executor.py ==="
cat cli/localci/core/executor.py
echo ""
echo "=== output.py ==="
cat cli/localci/utils/output.py
echo ""
echo "=== test_github_token.py ==="
cat cli/tests/test_github_token.py
echo ""
echo "=== test_executor.py ==="
cat cli/tests/test_executor.py
echo ""
echo "=== Directory structure ==="
tree cli/localci/core/ cli/localci/cli/ cli/localci/utils/ 2>/dev/null || find cli/localci -name "*.py" | sortLength of output: 114208
Overall AssessmentThe PR cleanly addresses issue Issues1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cli/tests/test_executor.py`:
- Around line 628-635: The test test_extract_error_ignores_401_false_positive
currently can pass for both correct and incorrect implementations; change the
mocked output and assertions so error-triggered extraction would yield a
different result than the fallback: feed _extract_error (on JobExecutor) an
output with at least 3 lines where the "progress: fetched 4010 bytes from cache"
line is not in the last two lines (e.g., put a trailing "all done" plus another
line after it), keep max_lines=2, then assert that the extracted text does not
contain "4010" (to prove the "401" substring did not trigger extraction) and
that it contains the expected fallback last-line content (e.g., "all done" or
the final line) to confirm fallback behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37164f43-4bc4-4710-9132-2043ad2e428e
📒 Files selected for processing (7)
cli/localci/cli/run.pycli/localci/core/command_builder.pycli/localci/core/executor.pycli/localci/core/github_token.pycli/localci/utils/output.pycli/tests/test_executor.pycli/tests/test_github_token.py
💤 Files with no reviewable changes (1)
- cli/tests/test_github_token.py
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/localci/core/github_token.py
- cli/localci/cli/run.py
- cli/localci/utils/output.py
- cli/localci/core/executor.py
- cli/localci/core/command_builder.py
henry0816191
left a comment
henry0816191
left a comment
There was a problem hiding this comment.
I approved this PR, but left a comment
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cli/tests/test_executor.py`:
- Around line 617-631: Update the two tests for JobExecutor._extract_error to
ensure they exercise the auth-extraction branch: craft longer multi-line output
so the fallback path cannot trivially capture the 401/403, and include one of
the literal auth keywords from AUTH_ERROR_EXTRACT_KEYWORDS (e.g.,
"unauthorized", "forbidden", or "rate limit") alongside the HTTP status line;
then assert that the returned extracted string contains both the numeric status
("401"/"403") and the auth keyword to verify the auth-specific extraction logic
in _extract_error.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27d15f32-10f4-4b13-b30e-dddf7ac9688f
📒 Files selected for processing (1)
cli/tests/test_executor.py
3 participants
Summary
Closes #44
Addresses the token slice of the Silent Failure Chain (T9 defaults + T10 error extraction): missing or placeholder
GITHUB_TOKENvalues previously produced opaque HTTP 401s from act with no upfront diagnostic.localci.core.github_tokenwithSENTINEL_GITHUB_TOKEN, resolution helpers, andwarn_sentinel_github_token()(Richprint_warningvia existing output helpers).localci runuses the sentinel, at orchestrator configuration time (dry-run plan and real execution paths), with remediation steps:export GITHUB_TOKEN,--github-token, or--offline.command_buildervia the shared constant.JobExecutor._extract_errorwith auth-related keywords (401,unauthorized,forbidden,rate limit) in an explicit_AUTH_ERROR_EXTRACT_KEYWORDStuple.Test plan
pytest tests/test_github_token.py— token resolution, sentinel detection, warning text, Rich warning output, auth keyword registration, CLI dry-run with/without tokenpytest tests/test_executor.py—_extract_errorfor build errors, HTTP 401, and default secrets sentinellocalci run --dry-runwithoutGITHUB_TOKEN→ yellow warning with setup instructionslocalci run --dry-run --github-token ghp_...→ no sentinel warningSummary by CodeRabbit
New Features
Bug Fixes
Tests