Repository navigation
Add build insights reporting tool and update dependencies - #9
Merged
Merged
Conversation
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
🧰 Additional context used🪛 Ruff (0.15.18)test/integration/report/test_get_build_insights_report.py[error] 123-123: Possible hardcoded password assigned to: "APPCIRCLE_ACCESS_TOKEN" (S105) src/tools/report/get_build_insights_report.py[warning] 78-78: Do not catch blind exception: (BLE001) [warning] 224-224: Add explicit value for parameter (B905) [warning] 294-294: Add explicit value for parameter (B905) [warning] 321-321: Add explicit value for parameter (B905) [warning] 324-324: Add explicit value for parameter (B905) [warning] 552-552: Do not catch blind exception: (BLE001) [warning] 553-553: Use explicit conversion flag Replace with conversion flag (RUF010) 🪛 zizmor (1.26.1).github/workflows/test.yml[warning] 11-29: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block (excessive-permissions) 📝 WalkthroughWalkthroughAdds a new Changesget_build_insights_report MCP Tool
Dependency and CI Updates
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
11-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLock this job down with explicit minimal permissions.
This audit job still runs with the workflow’s default
GITHUB_TOKENscope. Since it checks out code, installs packages, and runs third-party actions, add an explicit least-privilegepermissions:block here (for this job,contents: readis usually enough).Suggested fix
security-audit: + permissions: + contents: read runs-on: ubuntu-latest🤖 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 @.github/workflows/test.yml around lines 11 - 29, The security-audit job is still relying on the workflow default token scope; add an explicit least-privilege permissions block to this job so it only has the access it needs. Update the security-audit job definition in test.yml to set minimal permissions, with contents: read being sufficient for the checkout and audit steps, and leave the existing actions and commands unchanged.Source: Linters/SAST tools
🤖 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 `@pyproject.toml`:
- Around line 49-61: The security version pins are only defined under [tool.uv],
so they won’t be enforced by installer-agnostic paths like pip install . or pip
install -e .. Mirror the affected dependency constraints in the packaging
metadata used by the project’s install flow, and reference the existing
[tool.uv] constraint-dependencies list to keep the pinned versions consistent
across installers.
In `@src/tools/report/build_insights/artifact_health.py`:
- Around line 68-73: The baseline check in artifact health is using truthiness,
so a valid 0.0 previous average size is treated as missing data. Update the
logic in the artifact health calculation path, especially the delta computation
around prev_avg_size, to explicitly distinguish None from zero so 0.0 still
produces delta_mb and delta_pct values. Make sure biggest_grower_and_shrinker()
can then consider those profiles normally instead of skipping them.
In `@src/tools/report/build_insights/fetch.py`:
- Around line 35-41: Cap the pagination fan-out in fetch_all_build_insights and
fetch_all_queue_waiting by avoiding an unbounded asyncio.gather over every
remaining page. Replace the current one-request-per-page burst with batched
requests or a shared asyncio.Semaphore so only a limited number of
api_client.get calls run concurrently. Apply the same concurrency cap in both
pagination loops to keep get_build_insights_report from overwhelming Appcircle
when fetching large ranges.
In `@src/tools/report/build_insights/health_snapshot.py`:
- Around line 122-125: The build-insights classification in health_snapshot.py
is treating rows with missing triggerCategory as automated because the
manual/automated split in the build snapshot logic only checks for 0 vs != 0.
Update the logic around the current build list processing to exclude
unclassified rows before creating the manual and automated buckets, and make
sure the existing field_present gate in the snapshot function only allows data
through when triggerCategory is populated for the rows being counted.
In `@src/tools/report/build_insights/maturity_assessment.py`:
- Around line 132-145: The custom-script scoring in maturity_assessment.py
currently counts all top_profiles in the denominator even when a workflow could
not be fetched, which inflates the score for unresolved workflows. Update the
score calculation around git_scripts_failing/g_it_scripts_passing and
check_scores to exclude profiles without loaded workflow YAML from the
custom-script denominator, using the same resolved-profile population used by
the workflow-quality checks. Keep the logic aligned with the existing
top_profiles, workflow_quality_issues, and pr_configured_by_profile symbols so
only executed checks contribute to the score.
In `@src/tools/report/build_insights/root_cause.py`:
- Around line 120-123: The grouping logic in root_cause.py is only skipping
workflowName when it is None, but empty strings should be treated as missing
too. Update the workflow check in the build-insights aggregation path so the
existing by_combo append only runs when workflowName is a non-empty value,
matching the behavior of field_present(curr_builds, "workflowName") and
preventing workflow: "" cards from being created.
- Around line 45-68: The pct_of_failed baseline is wrong in root_cause because
failed builds without failedStep are filtered out before computing totals.
Update the logic in root_cause.py around failed_builds, step_counts, and
total_failed so the percentage is based on all failed_builds_all entries that
represent failures, while still only counting annotated steps for per-step
counts and combos. Keep the existing _split_failed_step and top-combo
aggregation, but compute the denominator from the full failed build set so
pct_of_failed reflects true prevalence.
In `@src/tools/report/build_insights/workflow_quality.py`:
- Around line 106-110: The returned payload in workflow_quality.py can become
inconsistent because total_score is computed by rounding the raw sum of
tier_scores.values() while tier1_score, tier2_score, and tier3_score are rounded
individually. Update the return logic in the workflow-quality scoring path so
total_score is derived from the already-rounded tier values, and keep the
calculation aligned with the existing CRITICAL, IMPORTANT, and ADVANCED tier
scoring symbols to avoid mismatched totals on Android.
- Around line 67-72: The detect_os helper is matching build records by
buildProfileName, which can collide when include_sub_orgs=True and cause
OS/workflow data to be assigned to the wrong profile. Update detect_os to use a
stable profile identity from the build record (the same unique identifier used
elsewhere in workflow_quality.py for profile lookups) instead of relying on the
mutable profile name, and adjust any callers or lookups in this flow so the
correct build history is selected.
In `@src/tools/report/build_insights/workflow_yaml.py`:
- Around line 150-163: The fallback logic in workflow resolution should not use
trigger-config `workflowId` when build history already resolved a side, even if
`find_workflow_id` fails to match that name in `/workflows/`. Update
`workflow_yaml.py` so `push_id` and `pr_id` only fall back when history did not
produce a workflow name at all, and keep history as the source of truth in the
workflow resolution path around `fetch_workflow_list`, `find_workflow_id`, and
`_first_configured_workflow_id`. Add a regression test covering the case where
history has `workflowName` but the workflows list omits that entry, and assert
it does not switch to the trigger-config workflow.
In `@src/tools/report/get_build_insights_report.py`:
- Around line 50-61: The inclusive window math in _resolve_period is off by one
day: the default and one-sided branches currently use DEFAULT_PERIOD_DAYS
directly, which produces a 31-day calendar span instead of the intended 30.
Update _resolve_period (including the branches that fill missing start_date or
end_date) to account for inclusive endpoints so the resolved period matches
_previous_period and the documented 30-day window.
- Around line 50-62: The _resolve_period helper currently accepts inverted
ranges, which lets start_date end up after end_date and later breaks
_previous_period with negative-duration math. Add an upfront validation in
_resolve_period to compare the resolved dates and reject cases where the start
is after the end, returning a clear user-facing error before any downstream
report generation or HTTP calls proceed.
- Around line 173-224: The build insights fetch flow in
get_build_insights_report should not require current build history when the only
requested section is queue_time. Update the async gather logic around
current_task and the optional_tasks in get_build_insights_report so
/report/v2/build is skipped when queue_time is the sole section, and ensure the
queue_waiting path can run independently using fetch_all_queue_waiting without
awaiting fetch_all_build_history first.
In `@test/integration/report/test_get_build_insights_report.py`:
- Around line 121-127: The invalid-token integration test is not asserting the
expected failure path because the check for an error only runs when
get_build_insights_report() returns a failure. Update the test around
get_build_insights_report() so it explicitly fails if success is returned with
the invalid APPCIRCLE_ACCESS_TOKEN, and only then validate that the error
payload contains the expected tool context; use the existing
get_build_insights_report() call and result.success/result.error fields to
locate the fix.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 11-29: The security-audit job is still relying on the workflow
default token scope; add an explicit least-privilege permissions block to this
job so it only has the access it needs. Update the security-audit job definition
in test.yml to set minimal permissions, with contents: read being sufficient for
the checkout and audit steps, and leave the existing actions and commands
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78dc37b3-03cd-486d-b82f-a7461624891b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.github/workflows/test.ymlREADME.mdpyproject.tomlsrc/core/api_client.pysrc/tools/report/__init__.pysrc/tools/report/build_insights/__init__.pysrc/tools/report/build_insights/artifact_health.pysrc/tools/report/build_insights/fetch.pysrc/tools/report/build_insights/health_snapshot.pysrc/tools/report/build_insights/maturity_assessment.pysrc/tools/report/build_insights/profile_configuration.pysrc/tools/report/build_insights/queue_time.pysrc/tools/report/build_insights/root_cause.pysrc/tools/report/build_insights/shared.pysrc/tools/report/build_insights/workflow_quality.pysrc/tools/report/build_insights/workflow_yaml.pysrc/tools/report/get_build_insights_report.pytest/integration/report/test_get_build_insights_report.pytest/unit/core/test_api_client.pytest/unit/tools/report/build_insights/__init__.pytest/unit/tools/report/build_insights/test_artifact_health.pytest/unit/tools/report/build_insights/test_fetch.pytest/unit/tools/report/build_insights/test_health_snapshot.pytest/unit/tools/report/build_insights/test_maturity_assessment.pytest/unit/tools/report/build_insights/test_profile_configuration.pytest/unit/tools/report/build_insights/test_queue_time.pytest/unit/tools/report/build_insights/test_root_cause.pytest/unit/tools/report/build_insights/test_shared.pytest/unit/tools/report/build_insights/test_workflow_quality.pytest/unit/tools/report/build_insights/test_workflow_yaml.pytest/unit/tools/report/test_get_build_insights_report.py
Comment on lines
+49
to
+61
| [tool.uv] | ||
| # Security-patched versions for indirect deps not declared in [project.dependencies]. | ||
| constraint-dependencies = [ | ||
| "authlib==1.6.12", | ||
| "cryptography==48.0.1", | ||
| "idna==3.15", | ||
| "msgpack==1.2.1", | ||
| "pip==26.1.2", | ||
| "pydantic-settings==2.14.2", | ||
| "pyjwt==2.13.0", | ||
| "python-multipart==0.0.31", | ||
| "urllib3==2.7.0", | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tool.uv constraints =="
sed -n '45,70p' pyproject.toml
echo
echo "== documented install / execution paths =="
rg -n --hidden \
--glob 'README*' \
--glob '.github/workflows/*.yml' \
--glob 'pyproject.toml' \
'pip install|python -m pip|uv sync|uv run|uv tool install'
echo
echo "== lockfile entries for constrained packages =="
rg -n 'authlib|cryptography|idna|msgpack|pip|pydantic-settings|pyjwt|python-multipart|urllib3' uv.lockRepository: appcircleio/appcircle-mcp
Length of output: 802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,200p'
echo
echo "== README / docs install hints =="
fd -i 'README*|*.md' . | xargs -r rg -n -H 'pip install|python -m pip|uv sync|uv run|poetry|hatch|setuptools|install'
echo
echo "== pyproject packaging metadata =="
sed -n '1,120p' pyproject.toml
echo
echo "== workflow install commands =="
fd -e yml -e yaml .github | xargs -r rg -n -H 'pip install|python -m pip|uv sync|uv run|uv tool install|actions/setup-python'Repository: appcircleio/appcircle-mcp
Length of output: 7985
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== README install references =="
rg -n -H --fixed-strings 'uv ' README.md docs .github/workflows/test.yml
echo
echo "== pip / installer references =="
rg -n -H 'pip install|python -m pip|uv sync|uv run|uv tool install|poetry|hatch' README.md docs .github/workflows/test.yml pyproject.toml
echo
echo "== pyproject metadata =="
sed -n '1,120p' pyproject.toml
echo
echo "== workflow file =="
sed -n '1,220p' .github/workflows/test.ymlRepository: appcircleio/appcircle-mcp
Length of output: 7507
Mirror these security pins outside [tool.uv]. pip install . and pip install -e . are documented install paths, so these transitive version pins won’t apply there. If these versions are part of the security baseline, enforce them in installer-agnostic packaging metadata as well.
🤖 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 `@pyproject.toml` around lines 49 - 61, The security version pins are only
defined under [tool.uv], so they won’t be enforced by installer-agnostic paths
like pip install . or pip install -e .. Mirror the affected dependency
constraints in the packaging metadata used by the project’s install flow, and
reference the existing [tool.uv] constraint-dependencies list to keep the pinned
versions consistent across installers.
Comment on lines
+68
to
+73
| if prev_avg_size: | ||
| delta_mb = current_avg_size - prev_avg_size | ||
| delta_pct = delta_mb / prev_avg_size * 100 | ||
| else: | ||
| delta_mb = None | ||
| delta_pct = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't treat a 0.0 baseline as missing data.
Line 68 uses truthiness, so a previous average size of 0.0 falls into the “no baseline” branch. That drops a valid delta_mb and also prevents this profile from ever qualifying in biggest_grower_and_shrinker().
Suggested fix
- if prev_avg_size:
+ if prev_avg_size is not None:
delta_mb = current_avg_size - prev_avg_size
- delta_pct = delta_mb / prev_avg_size * 100
+ delta_pct = None if prev_avg_size == 0 else delta_mb / prev_avg_size * 100
else:
delta_mb = None
delta_pct = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if prev_avg_size: | |
| delta_mb = current_avg_size - prev_avg_size | |
| delta_pct = delta_mb / prev_avg_size * 100 | |
| else: | |
| delta_mb = None | |
| delta_pct = None | |
| if prev_avg_size is not None: | |
| delta_mb = current_avg_size - prev_avg_size | |
| delta_pct = None if prev_avg_size == 0 else delta_mb / prev_avg_size * 100 | |
| else: | |
| delta_mb = None | |
| delta_pct = None |
🤖 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 `@src/tools/report/build_insights/artifact_health.py` around lines 68 - 73, The
baseline check in artifact health is using truthiness, so a valid 0.0 previous
average size is treated as missing data. Update the logic in the artifact health
calculation path, especially the delta computation around prev_avg_size, to
explicitly distinguish None from zero so 0.0 still produces delta_mb and
delta_pct values. Make sure biggest_grower_and_shrinker() can then consider
those profiles normally instead of skipping them.
Comment on lines
+35
to
+41
| if total_pages > 1: | ||
| pages = await asyncio.gather(*[ | ||
| api_client.get("/report/v2/build", params={**params, "page": str(page)}) | ||
| for page in range(2, total_pages + 1) | ||
| ]) | ||
| for page_response in pages: | ||
| records.extend(page_response.get("data", [])) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cap the pagination fan-out.
Both loops fire one request per remaining page in a single asyncio.gather(). get_build_insights_report can already run multiple history fetches in parallel, so a large date range can burst hundreds of Appcircle calls at once and turn one report request into a rate-limit/connection-pool problem. Batch these pages or gate them with a semaphore.
Possible fix
- if total_pages > 1:
- pages = await asyncio.gather(*[
- api_client.get("/report/v2/build", params={**params, "page": str(page)})
- for page in range(2, total_pages + 1)
- ])
- for page_response in pages:
- records.extend(page_response.get("data", []))
+ if total_pages > 1:
+ for batch_start in range(2, total_pages + 1, 20):
+ batch_end = min(batch_start + 20, total_pages + 1)
+ pages = await asyncio.gather(*[
+ api_client.get("/report/v2/build", params={**params, "page": str(page)})
+ for page in range(batch_start, batch_end)
+ ])
+ for page_response in pages:
+ records.extend(page_response.get("data", []))Apply the same cap to fetch_all_queue_waiting.
Also applies to: 76-82
🤖 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 `@src/tools/report/build_insights/fetch.py` around lines 35 - 41, Cap the
pagination fan-out in fetch_all_build_insights and fetch_all_queue_waiting by
avoiding an unbounded asyncio.gather over every remaining page. Replace the
current one-request-per-page burst with batched requests or a shared
asyncio.Semaphore so only a limited number of api_client.get calls run
concurrently. Apply the same concurrency cap in both pagination loops to keep
get_build_insights_report from overwhelming Appcircle when fetching large
ranges.
Comment on lines
+122
to
+125
| if not field_present(curr_builds, "triggerCategory") or not field_present(curr_builds, "initiatedBy"): | ||
| return None | ||
| manual = [b for b in curr_builds if b.get("triggerCategory") == 0] | ||
| automated = [b for b in curr_builds if b.get("triggerCategory") != 0] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't classify missing triggerCategory values as automated.
Lines 124-125 only need one populated triggerCategory to pass the gate, then any row with a missing value is counted as automated via != 0. That fabricates trigger counts when the API returns mixed-population data. Filter out unclassified rows before splitting.
Proposed fix
- manual = [b for b in curr_builds if b.get("triggerCategory") == 0]
- automated = [b for b in curr_builds if b.get("triggerCategory") != 0]
+ classified_builds = [b for b in curr_builds if b.get("triggerCategory") is not None]
+ manual = [b for b in classified_builds if b["triggerCategory"] == 0]
+ automated = [b for b in classified_builds if b["triggerCategory"] != 0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not field_present(curr_builds, "triggerCategory") or not field_present(curr_builds, "initiatedBy"): | |
| return None | |
| manual = [b for b in curr_builds if b.get("triggerCategory") == 0] | |
| automated = [b for b in curr_builds if b.get("triggerCategory") != 0] | |
| if not field_present(curr_builds, "triggerCategory") or not field_present(curr_builds, "initiatedBy"): | |
| return None | |
| classified_builds = [b for b in curr_builds if b.get("triggerCategory") is not None] | |
| manual = [b for b in classified_builds if b["triggerCategory"] == 0] | |
| automated = [b for b in classified_builds if b["triggerCategory"] != 0] |
🤖 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 `@src/tools/report/build_insights/health_snapshot.py` around lines 122 - 125,
The build-insights classification in health_snapshot.py is treating rows with
missing triggerCategory as automated because the manual/automated split in the
build snapshot logic only checks for 0 vs != 0. Update the logic around the
current build list processing to exclude unclassified rows before creating the
manual and automated buckets, and make sure the existing field_present gate in
the snapshot function only allows data through when triggerCategory is populated
for the rows being counted.
Comment on lines
+132
to
+145
| git_scripts_failing = { | ||
| issue["profile"] for issue in workflow_quality_issues if issue["check"] == "custom_script_not_from_git" | ||
| } | ||
| git_scripts_passing = len(top_profiles) - len(git_scripts_failing) | ||
|
|
||
| # `None` (unresolved profile) is excluded from the applicable population, not scored as unconfigured. | ||
| pr_check_applicable = [p for p in top_profiles if pr_configured_by_profile.get(p) is not None] | ||
| pr_configured_passing = sum(1 for p in pr_check_applicable if pr_configured_by_profile.get(p)) | ||
|
|
||
| check_scores = [ | ||
| (versioning_passing / versioning_applicable) * (100 / 3) if versioning_applicable else 0.0, | ||
| (git_scripts_passing / len(top_profiles)) * (100 / 3) if top_profiles else 0.0, | ||
| (pr_configured_passing / len(pr_check_applicable)) * (100 / 3) if pr_check_applicable else 0.0, | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude unresolved workflows from the custom-script denominator.
git_scripts_passing = len(top_profiles) - len(git_scripts_failing) treats every profile with missing workflow YAML as a pass. Under the tool's graceful-degradation path, a failed workflow fetch will inflate bp_score and the discipline score even though this check never executed for that profile.
Proposed fix
- git_scripts_failing = {
- issue["profile"] for issue in workflow_quality_issues if issue["check"] == "custom_script_not_from_git"
- }
- git_scripts_passing = len(top_profiles) - len(git_scripts_failing)
+ git_scripts_applicable = {
+ row["profile"]
+ for row in per_profile_quality
+ if row.get("push") or row.get("pr")
+ }
+ git_scripts_failing = {
+ issue["profile"]
+ for issue in workflow_quality_issues
+ if issue["check"] == "custom_script_not_from_git"
+ }
+ git_scripts_passing = len(git_scripts_applicable - git_scripts_failing)
@@
- (git_scripts_passing / len(top_profiles)) * (100 / 3) if top_profiles else 0.0,
+ (git_scripts_passing / len(git_scripts_applicable)) * (100 / 3)
+ if git_scripts_applicable else 0.0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| git_scripts_failing = { | |
| issue["profile"] for issue in workflow_quality_issues if issue["check"] == "custom_script_not_from_git" | |
| } | |
| git_scripts_passing = len(top_profiles) - len(git_scripts_failing) | |
| # `None` (unresolved profile) is excluded from the applicable population, not scored as unconfigured. | |
| pr_check_applicable = [p for p in top_profiles if pr_configured_by_profile.get(p) is not None] | |
| pr_configured_passing = sum(1 for p in pr_check_applicable if pr_configured_by_profile.get(p)) | |
| check_scores = [ | |
| (versioning_passing / versioning_applicable) * (100 / 3) if versioning_applicable else 0.0, | |
| (git_scripts_passing / len(top_profiles)) * (100 / 3) if top_profiles else 0.0, | |
| (pr_configured_passing / len(pr_check_applicable)) * (100 / 3) if pr_check_applicable else 0.0, | |
| ] | |
| git_scripts_applicable = { | |
| row["profile"] | |
| for row in per_profile_quality | |
| if row.get("push") or row.get("pr") | |
| } | |
| git_scripts_failing = { | |
| issue["profile"] for issue in workflow_quality_issues if issue["check"] == "custom_script_not_from_git" | |
| } | |
| git_scripts_passing = len(git_scripts_applicable - git_scripts_failing) | |
| # `None` (unresolved profile) is excluded from the applicable population, not scored as unconfigured. | |
| pr_check_applicable = [p for p in top_profiles if pr_configured_by_profile.get(p) is not None] | |
| pr_configured_passing = sum(1 for p in pr_check_applicable if pr_configured_by_profile.get(p)) | |
| check_scores = [ | |
| (versioning_passing / versioning_applicable) * (100 / 3) if versioning_applicable else 0.0, | |
| (git_scripts_passing / len(git_scripts_applicable)) * (100 / 3) | |
| if git_scripts_applicable else 0.0, | |
| (pr_configured_passing / len(pr_check_applicable)) * (100 / 3) if pr_check_applicable else 0.0, | |
| ] |
🤖 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 `@src/tools/report/build_insights/maturity_assessment.py` around lines 132 -
145, The custom-script scoring in maturity_assessment.py currently counts all
top_profiles in the denominator even when a workflow could not be fetched, which
inflates the score for unresolved workflows. Update the score calculation around
git_scripts_failing/g_it_scripts_passing and check_scores to exclude profiles
without loaded workflow YAML from the custom-script denominator, using the same
resolved-profile population used by the workflow-quality checks. Keep the logic
aligned with the existing top_profiles, workflow_quality_issues, and
pr_configured_by_profile symbols so only executed checks contribute to the
score.
Comment on lines
+150
to
+163
| push_id = pr_id = None | ||
| if push_name or pr_name: | ||
| workflows = await fetch_workflow_list(api_client, profile_id) | ||
| push_id = find_workflow_id(workflows, push_name) if push_name else None | ||
| pr_id = find_workflow_id(workflows, pr_name) if pr_name else None | ||
|
|
||
| # Fall back to the trigger-config endpoint's `workflowId` for whichever | ||
| # side build history didn't resolve. History takes priority when it | ||
| # already resolved a side. | ||
| if triggers: | ||
| if push_id is None: | ||
| push_id = _first_configured_workflow_id(triggers.get("pushSettings")) | ||
| if pr_id is None: | ||
| pr_id = _first_configured_workflow_id(triggers.get("pullRequestSettings")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not fall back once build history already identified that side.
push_id is None / pr_id is None currently conflates “history never resolved a workflow name” with “history resolved a name, but /workflows/ did not return a matching id”. In the second case this code silently swaps in the trigger-config workflow, even though the docstring and tests say history must win. That will score the wrong Push/PR workflow whenever the workflow list is stale, partial, or filtered.
Suggested fix
push_name, pr_name = resolve_push_pr_workflow_names(curr_builds, profile)
push_id = pr_id = None
if push_name or pr_name:
workflows = await fetch_workflow_list(api_client, profile_id)
push_id = find_workflow_id(workflows, push_name) if push_name else None
pr_id = find_workflow_id(workflows, pr_name) if pr_name else None
# Fall back to the trigger-config endpoint's `workflowId` for whichever
# side build history didn't resolve. History takes priority when it
# already resolved a side.
if triggers:
- if push_id is None:
+ if push_name is None and push_id is None:
push_id = _first_configured_workflow_id(triggers.get("pushSettings"))
- if pr_id is None:
+ if pr_name is None and pr_id is None:
pr_id = _first_configured_workflow_id(triggers.get("pullRequestSettings"))Please add a regression test for the case where history resolves workflowName but /workflows/ omits that entry.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| push_id = pr_id = None | |
| if push_name or pr_name: | |
| workflows = await fetch_workflow_list(api_client, profile_id) | |
| push_id = find_workflow_id(workflows, push_name) if push_name else None | |
| pr_id = find_workflow_id(workflows, pr_name) if pr_name else None | |
| # Fall back to the trigger-config endpoint's `workflowId` for whichever | |
| # side build history didn't resolve. History takes priority when it | |
| # already resolved a side. | |
| if triggers: | |
| if push_id is None: | |
| push_id = _first_configured_workflow_id(triggers.get("pushSettings")) | |
| if pr_id is None: | |
| pr_id = _first_configured_workflow_id(triggers.get("pullRequestSettings")) | |
| push_id = pr_id = None | |
| if push_name or pr_name: | |
| workflows = await fetch_workflow_list(api_client, profile_id) | |
| push_id = find_workflow_id(workflows, push_name) if push_name else None | |
| pr_id = find_workflow_id(workflows, pr_name) if pr_name else None | |
| # Fall back to the trigger-config endpoint's `workflowId` for whichever | |
| # side build history didn't resolve. History takes priority when it | |
| # already resolved a side. | |
| if triggers: | |
| if push_name is None and push_id is None: | |
| push_id = _first_configured_workflow_id(triggers.get("pushSettings")) | |
| if pr_name is None and pr_id is None: | |
| pr_id = _first_configured_workflow_id(triggers.get("pullRequestSettings")) |
🤖 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 `@src/tools/report/build_insights/workflow_yaml.py` around lines 150 - 163, The
fallback logic in workflow resolution should not use trigger-config `workflowId`
when build history already resolved a side, even if `find_workflow_id` fails to
match that name in `/workflows/`. Update `workflow_yaml.py` so `push_id` and
`pr_id` only fall back when history did not produce a workflow name at all, and
keep history as the source of truth in the workflow resolution path around
`fetch_workflow_list`, `find_workflow_id`, and `_first_configured_workflow_id`.
Add a regression test covering the case where history has `workflowName` but the
workflows list omits that entry, and assert it does not switch to the
trigger-config workflow.
Comment on lines
+50
to
+61
| def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple: | ||
| """Both omitted -> last 30 days. Only one given -> fill the other to a | ||
| 30-day-wide window. Both given -> used exactly as provided.""" | ||
| if start_date is None and end_date is None: | ||
| end = date.today() | ||
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end | ||
| if end_date is None: | ||
| start = date.fromisoformat(start_date) | ||
| return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS), date.today()) | ||
| if start_date is None: | ||
| end = date.fromisoformat(end_date) | ||
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the inclusive date-window math.
_previous_period() treats the range as inclusive, but _resolve_period() subtracts/adds DEFAULT_PERIOD_DAYS directly. That makes the default and one-sided presets span 31 calendar days instead of the documented 30, so every default comparison window is off by one day.
Suggested fix
def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple:
"""Both omitted -> last 30 days. Only one given -> fill the other to a
30-day-wide window. Both given -> used exactly as provided."""
if start_date is None and end_date is None:
end = date.today()
- return end - timedelta(days=DEFAULT_PERIOD_DAYS), end
+ return end - timedelta(days=DEFAULT_PERIOD_DAYS - 1), end
if end_date is None:
start = date.fromisoformat(start_date)
- return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS), date.today())
+ return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS - 1), date.today())
if start_date is None:
end = date.fromisoformat(end_date)
- return end - timedelta(days=DEFAULT_PERIOD_DAYS), end
+ return end - timedelta(days=DEFAULT_PERIOD_DAYS - 1), end
return date.fromisoformat(start_date), date.fromisoformat(end_date)Also applies to: 65-69
🤖 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 `@src/tools/report/get_build_insights_report.py` around lines 50 - 61, The
inclusive window math in _resolve_period is off by one day: the default and
one-sided branches currently use DEFAULT_PERIOD_DAYS directly, which produces a
31-day calendar span instead of the intended 30. Update _resolve_period
(including the branches that fill missing start_date or end_date) to account for
inclusive endpoints so the resolved period matches _previous_period and the
documented 30-day window.
Comment on lines
+50
to
+62
| def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple: | ||
| """Both omitted -> last 30 days. Only one given -> fill the other to a | ||
| 30-day-wide window. Both given -> used exactly as provided.""" | ||
| if start_date is None and end_date is None: | ||
| end = date.today() | ||
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end | ||
| if end_date is None: | ||
| start = date.fromisoformat(start_date) | ||
| return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS), date.today()) | ||
| if start_date is None: | ||
| end = date.fromisoformat(end_date) | ||
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end | ||
| return date.fromisoformat(start_date), date.fromisoformat(end_date) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject inverted date ranges up front.
If start_date is after end_date, this helper still returns the pair and _previous_period() derives a negative-duration baseline from it. That turns a user validation error into incoherent date math and downstream HTTP behavior.
Suggested fix
def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple:
@@
- if start_date is None and end_date is None:
- end = date.today()
- return end - timedelta(days=DEFAULT_PERIOD_DAYS), end
- if end_date is None:
- start = date.fromisoformat(start_date)
- return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS), date.today())
- if start_date is None:
- end = date.fromisoformat(end_date)
- return end - timedelta(days=DEFAULT_PERIOD_DAYS), end
- return date.fromisoformat(start_date), date.fromisoformat(end_date)
+ if start_date is None and end_date is None:
+ end = date.today()
+ start = end - timedelta(days=DEFAULT_PERIOD_DAYS - 1)
+ elif end_date is None:
+ start = date.fromisoformat(start_date)
+ end = min(start + timedelta(days=DEFAULT_PERIOD_DAYS - 1), date.today())
+ elif start_date is None:
+ end = date.fromisoformat(end_date)
+ start = end - timedelta(days=DEFAULT_PERIOD_DAYS - 1)
+ else:
+ start = date.fromisoformat(start_date)
+ end = date.fromisoformat(end_date)
+
+ if start > end:
+ raise ValueError("start_date must be on or before end_date")
+ return start, end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple: | |
| """Both omitted -> last 30 days. Only one given -> fill the other to a | |
| 30-day-wide window. Both given -> used exactly as provided.""" | |
| if start_date is None and end_date is None: | |
| end = date.today() | |
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end | |
| if end_date is None: | |
| start = date.fromisoformat(start_date) | |
| return start, min(start + timedelta(days=DEFAULT_PERIOD_DAYS), date.today()) | |
| if start_date is None: | |
| end = date.fromisoformat(end_date) | |
| return end - timedelta(days=DEFAULT_PERIOD_DAYS), end | |
| return date.fromisoformat(start_date), date.fromisoformat(end_date) | |
| def _resolve_period(start_date: Optional[str], end_date: Optional[str]) -> tuple: | |
| """Both omitted -> last 30 days. Only one given -> fill the other to a | |
| 30-day-wide window. Both given -> used exactly as provided.""" | |
| if start_date is None and end_date is None: | |
| end = date.today() | |
| start = end - timedelta(days=DEFAULT_PERIOD_DAYS - 1) | |
| elif end_date is None: | |
| start = date.fromisoformat(start_date) | |
| end = min(start + timedelta(days=DEFAULT_PERIOD_DAYS - 1), date.today()) | |
| elif start_date is None: | |
| end = date.fromisoformat(end_date) | |
| start = end - timedelta(days=DEFAULT_PERIOD_DAYS - 1) | |
| else: | |
| start = date.fromisoformat(start_date) | |
| end = date.fromisoformat(end_date) | |
| if start > end: | |
| raise ValueError("start_date must be on or before end_date") | |
| return start, end |
🤖 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 `@src/tools/report/get_build_insights_report.py` around lines 50 - 62, The
_resolve_period helper currently accepts inverted ranges, which lets start_date
end up after end_date and later breaks _previous_period with negative-duration
math. Add an upfront validation in _resolve_period to compare the resolved dates
and reject cases where the start is after the end, returning a clear user-facing
error before any downstream report generation or HTTP calls proceed.
Comment on lines
+173
to
+224
| async with ApiClient() as api_client: | ||
| current_task = fetch_all_build_history( | ||
| api_client, | ||
| start_date=current_start.isoformat(), | ||
| end_date=current_end.isoformat(), | ||
| ) | ||
|
|
||
| # Each fetch below is wrapped via _soft_fetch, degrading to a default | ||
| # value on failure instead of raising out of the gather. | ||
| optional_tasks: Dict[str, Any] = {} | ||
| if needs_previous_period: | ||
| optional_tasks["previous"] = _soft_fetch( | ||
| "previous_period", | ||
| fetch_all_build_history( | ||
| api_client, | ||
| start_date=previous_start.isoformat(), | ||
| end_date=previous_end.isoformat(), | ||
| ), | ||
| [], | ||
| failed_fetches, | ||
| ) | ||
| if needs_profiles: | ||
| optional_tasks["profiles"] = _soft_fetch( | ||
| "profiles", | ||
| fetch_all_build_profiles(api_client), | ||
| None, | ||
| failed_fetches, | ||
| ) | ||
| if needs_queue_time: | ||
| optional_tasks["queue_waiting"] = _soft_fetch( | ||
| "queue_waiting", | ||
| fetch_all_queue_waiting( | ||
| api_client, | ||
| start_date=current_start.isoformat(), | ||
| end_date=current_end.isoformat(), | ||
| ), | ||
| {"records": [], "summary": None}, | ||
| failed_fetches, | ||
| ) | ||
| if needs_maturity_assessment: | ||
| optional_tasks["certificates"] = _soft_fetch( | ||
| "certificates", fetch_certificates(api_client), None, failed_fetches | ||
| ) | ||
| optional_tasks["provisioning_profiles"] = _soft_fetch( | ||
| "provisioning_profiles", fetch_provisioning_profiles(api_client), None, failed_fetches | ||
| ) | ||
| optional_tasks["keystores"] = _soft_fetch( | ||
| "keystores", fetch_keystores(api_client), None, failed_fetches | ||
| ) | ||
|
|
||
| current_result, *optional_results = await asyncio.gather(current_task, *optional_tasks.values()) | ||
| results = {"current": current_result, **dict(zip(optional_tasks.keys(), optional_results))} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip /report/v2/build when queue_time is the only requested section.
queue_time is computed entirely from /report/v1/build/queue-waiting, but this path always awaits current_task first. A sections=["queue_time"] call can therefore fail on an unrelated build-history outage or permission error, and it always pays for an extra request it never uses.
Suggested fix
failed_fetches: List[str] = []
+ needs_current_history = requested_sections != {"queue_time"}
async with ApiClient() as api_client:
- current_task = fetch_all_build_history(
- api_client,
- start_date=current_start.isoformat(),
- end_date=current_end.isoformat(),
- )
+ current_task = (
+ fetch_all_build_history(
+ api_client,
+ start_date=current_start.isoformat(),
+ end_date=current_end.isoformat(),
+ )
+ if needs_current_history
+ else None
+ )
@@
- current_result, *optional_results = await asyncio.gather(current_task, *optional_tasks.values())
- results = {"current": current_result, **dict(zip(optional_tasks.keys(), optional_results))}
+ if current_task is not None:
+ current_result, *optional_results = await asyncio.gather(current_task, *optional_tasks.values())
+ else:
+ current_result = []
+ optional_results = await asyncio.gather(*optional_tasks.values())
+ results = {"current": current_result, **dict(zip(optional_tasks.keys(), optional_results))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async with ApiClient() as api_client: | |
| current_task = fetch_all_build_history( | |
| api_client, | |
| start_date=current_start.isoformat(), | |
| end_date=current_end.isoformat(), | |
| ) | |
| # Each fetch below is wrapped via _soft_fetch, degrading to a default | |
| # value on failure instead of raising out of the gather. | |
| optional_tasks: Dict[str, Any] = {} | |
| if needs_previous_period: | |
| optional_tasks["previous"] = _soft_fetch( | |
| "previous_period", | |
| fetch_all_build_history( | |
| api_client, | |
| start_date=previous_start.isoformat(), | |
| end_date=previous_end.isoformat(), | |
| ), | |
| [], | |
| failed_fetches, | |
| ) | |
| if needs_profiles: | |
| optional_tasks["profiles"] = _soft_fetch( | |
| "profiles", | |
| fetch_all_build_profiles(api_client), | |
| None, | |
| failed_fetches, | |
| ) | |
| if needs_queue_time: | |
| optional_tasks["queue_waiting"] = _soft_fetch( | |
| "queue_waiting", | |
| fetch_all_queue_waiting( | |
| api_client, | |
| start_date=current_start.isoformat(), | |
| end_date=current_end.isoformat(), | |
| ), | |
| {"records": [], "summary": None}, | |
| failed_fetches, | |
| ) | |
| if needs_maturity_assessment: | |
| optional_tasks["certificates"] = _soft_fetch( | |
| "certificates", fetch_certificates(api_client), None, failed_fetches | |
| ) | |
| optional_tasks["provisioning_profiles"] = _soft_fetch( | |
| "provisioning_profiles", fetch_provisioning_profiles(api_client), None, failed_fetches | |
| ) | |
| optional_tasks["keystores"] = _soft_fetch( | |
| "keystores", fetch_keystores(api_client), None, failed_fetches | |
| ) | |
| current_result, *optional_results = await asyncio.gather(current_task, *optional_tasks.values()) | |
| results = {"current": current_result, **dict(zip(optional_tasks.keys(), optional_results))} | |
| failed_fetches: List[str] = [] | |
| needs_current_history = requested_sections != {"queue_time"} | |
| async with ApiClient() as api_client: | |
| current_task = ( | |
| fetch_all_build_history( | |
| api_client, | |
| start_date=current_start.isoformat(), | |
| end_date=current_end.isoformat(), | |
| ) | |
| if needs_current_history | |
| else None | |
| ) | |
| # Each fetch below is wrapped via _soft_fetch, degrading to a default | |
| # value on failure instead of raising out of the gather. | |
| optional_tasks: Dict[str, Any] = {} | |
| if needs_previous_period: | |
| optional_tasks["previous"] = _soft_fetch( | |
| "previous_period", | |
| fetch_all_build_history( | |
| api_client, | |
| start_date=previous_start.isoformat(), | |
| end_date=previous_end.isoformat(), | |
| ), | |
| [], | |
| failed_fetches, | |
| ) | |
| if needs_profiles: | |
| optional_tasks["profiles"] = _soft_fetch( | |
| "profiles", | |
| fetch_all_build_profiles(api_client), | |
| None, | |
| failed_fetches, | |
| ) | |
| if needs_queue_time: | |
| optional_tasks["queue_waiting"] = _soft_fetch( | |
| "queue_waiting", | |
| fetch_all_queue_waiting( | |
| api_client, | |
| start_date=current_start.isoformat(), | |
| end_date=current_end.isoformat(), | |
| ), | |
| {"records": [], "summary": None}, | |
| failed_fetches, | |
| ) | |
| if needs_maturity_assessment: | |
| optional_tasks["certificates"] = _soft_fetch( | |
| "certificates", fetch_certificates(api_client), None, failed_fetches | |
| ) | |
| optional_tasks["provisioning_profiles"] = _soft_fetch( | |
| "provisioning_profiles", fetch_provisioning_profiles(api_client), None, failed_fetches | |
| ) | |
| optional_tasks["keystores"] = _soft_fetch( | |
| "keystores", fetch_keystores(api_client), None, failed_fetches | |
| ) | |
| if current_task is not None: | |
| current_result, *optional_results = await asyncio.gather(current_task, *optional_tasks.values()) | |
| else: | |
| current_result = [] | |
| optional_results = await asyncio.gather(*optional_tasks.values()) | |
| results = {"current": current_result, **dict(zip(optional_tasks.keys(), optional_results))} |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 224-224: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@src/tools/report/get_build_insights_report.py` around lines 173 - 224, The
build insights fetch flow in get_build_insights_report should not require
current build history when the only requested section is queue_time. Update the
async gather logic around current_task and the optional_tasks in
get_build_insights_report so /report/v2/build is skipped when queue_time is the
sole section, and ensure the queue_waiting path can run independently using
fetch_all_queue_waiting without awaiting fetch_all_build_history first.
Comment on lines
+121
to
+127
| original = settings.APPCIRCLE_ACCESS_TOKEN | ||
| try: | ||
| settings.APPCIRCLE_ACCESS_TOKEN = "invalid" | ||
| result = await get_build_insights_report() | ||
| if not result.get("success"): | ||
| assert "error" in result and "tool" in result["error"] | ||
| finally: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the invalid-token integration test fail closed.
This test currently passes when get_build_insights_report() unexpectedly succeeds, because the assertion only runs inside if not result.get("success"). That leaves the auth failure path unverified.
Suggested fix
original = settings.APPCIRCLE_ACCESS_TOKEN
try:
settings.APPCIRCLE_ACCESS_TOKEN = "invalid"
result = await get_build_insights_report()
- if not result.get("success"):
- assert "error" in result and "tool" in result["error"]
+ assert result.get("success") is False
+ assert "error" in result and "tool" in result["error"]
finally:
settings.APPCIRCLE_ACCESS_TOKEN = os.getenv("APPCIRCLE_ACCESS_TOKEN") or original📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| original = settings.APPCIRCLE_ACCESS_TOKEN | |
| try: | |
| settings.APPCIRCLE_ACCESS_TOKEN = "invalid" | |
| result = await get_build_insights_report() | |
| if not result.get("success"): | |
| assert "error" in result and "tool" in result["error"] | |
| finally: | |
| original = settings.APPCIRCLE_ACCESS_TOKEN | |
| try: | |
| settings.APPCIRCLE_ACCESS_TOKEN = "invalid" | |
| result = await get_build_insights_report() | |
| assert result.get("success") is False | |
| assert "error" in result and "tool" in result["error"] | |
| finally: |
🧰 Tools
🪛 Ruff (0.15.18)
[error] 123-123: Possible hardcoded password assigned to: "APPCIRCLE_ACCESS_TOKEN"
(S105)
🤖 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 `@test/integration/report/test_get_build_insights_report.py` around lines 121 -
127, The invalid-token integration test is not asserting the expected failure
path because the check for an error only runs when get_build_insights_report()
returns a failure. Update the test around get_build_insights_report() so it
explicitly fails if success is returned with the invalid APPCIRCLE_ACCESS_TOKEN,
and only then validate that the error payload contains the expected tool
context; use the existing get_build_insights_report() call and
result.success/result.error fields to locate the fix.
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
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.
Summary by CodeRabbit
New Features
Bug Fixes