Conversation
Add CI workflow that runs the OIDC Basic Certification test plan using pre-built conformance suite images from GHCR. Includes a separate workflow_dispatch to build and publish the conformance suite images. Passes 21/34 conformance tests. Assisted-by: Claude Code (model: claude-opus-4-6)
CONFORMANCE_HTTPD_IMAGE and CONFORMANCE_SERVER_IMAGE were only defined in the "Start conformance suite" step env. Other steps calling docker compose (logs in wait step and failure diagnostics) lacked these variables, causing compose to fail with "service has neither an image nor a build context". Assisted-by: Claude Code (model: claude-opus-4-6)
The previous commit moved image env vars to job-level env using
${{ env.X }}, but the env context is not available at job level.
Define full image names directly at workflow level instead.
Assisted-by: Claude Code (model: claude-opus-4-6)
- Add --expected-skips support to skip specified test modules - Add test result statistics (passed/failed/skipped counts) - Use jq instead of python3 for JSON parsing in run.sh - Move JVM property before -jar in docker-compose command - Use MYSQL_PWD env var instead of -p flag - Remove unnecessary f-string prefixes - Add SSL verification comment for clarity Assisted-by: Claude Code (model: claude-opus-4-6)
Fixes PATH hijacking vulnerability in go.opentelemetry.io/otel/sdk. Assisted-by: Claude Code (model: claude-opus-4-6)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughOIDCコンフォーマンステスト基盤を追加します。Docker Compose定義と設定テンプレート、スキップリスト、クライアント生成とテスト実行を自動化する Bash/Python スクリプト、及び GitHub Actions ワークフローと成果物除外を追加します。 Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as "GitHub Actions"
participant Portal as "Portal OIDC\n(Server)"
participant Suite as "Conformance\nTest Suite"
participant DB as "MongoDB"
participant Browser as "Browser\n(Client)"
GHA->>DB: 起動・スキーマ適用
GHA->>Portal: Portal 起動 (env, keys)
GHA->>Suite: Docker ComposeでSuite起動
GHA->>Portal: POST /admin/clients (create test client)
Portal-->>GHA: client_id, client_secret
GHA->>Suite: POST /api/plan (config + creds)
Suite-->>GHA: plan_id
loop 各モジュール
GHA->>Suite: POST /start (module)
Suite->>Portal: Authorization リクエスト
Portal-->>Browser: 認証フロー (ユーザ操作)
Browser->>Suite: callback/fragment 提出
Suite->>GHA: モジュールログ/状態
end
GHA->>GHA: 結果収集・アップロード
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/conformance/run.sh:
- Around line 28-33: The CLIENT_ID and CLIENT_SECRET extraction using jq -r can
yield the literal "null" which bypasses your empty-check; update the logic
around RESPONSE, CLIENT_ID and CLIENT_SECRET so that extraction treats both
empty string and the string "null" as failures (or use jq -e to make jq fail on
null) and then exit with error if either CLIENT_ID or CLIENT_SECRET is empty or
"null"; adjust the checks that currently reference CLIENT_ID and CLIENT_SECRET
to explicitly reject both "" and "null".
- Around line 39-44: The sed-based substitution (the multi-line sed that
replaces ${DISCOVERY_URL}, ${CLIENT_ID}, ${CLIENT_SECRET} into
config.template.json) is unsafe for values containing &, \, or other special
chars; replace the sed approach with a JSON-aware assignment using a tool like
jq: read the template (config.template.json), set the keys for DISCOVERY_URL,
CLIENT_ID and CLIENT_SECRET from the environment variables (DISCOVERY_URL,
CLIENT_ID, CLIENT_SECRET) with proper JSON escaping, and write the result to the
same output file ("results/config.json")—locate the sed invocation and the
variables DISCOVERY_URL/CLIENT_ID/CLIENT_SECRET in the script and swap it for a
jq-based read/update/write sequence to ensure correct escaping.
In @.github/scripts/run-test-plan.py:
- Around line 215-216: The script currently treats the "REVIEW" result as a
passing status which hides modules requiring manual verification; modify the
logic around the variable result and the all_passed flag so that only ("PASSED",
"WARNING", "SKIPPED") count as passes (remove "REVIEW"), and update any summary
counting/aggregation that groups results (the blocks around the checks at lines
using result, all_passed and the summary accumulation that currently includes
"REVIEW") so that "REVIEW" is tracked separately (e.g., its own counter or
category) and causes a non-zero exit (or at least not counted as success) so CI
reflects manual-review items; ensure references to result, all_passed and the
summary aggregation are changed consistently (both the initial pass check and
the later summary/exit handling) to exclude "REVIEW" from the pass set.
- Around line 110-125: The test runner opens callback_url then posts an empty
body to implicit_url, but fragments (the `#fragment`) are not sent in HTTP
requests so the implicit/hybrid response isn't reproduced; update the flow in
the block handling callback_url/implicit_url so you (1) parse the fragment out
of callback_url (use urlsplit to extract fragment) and include that fragment as
the POST body when calling browser.post(implicit_url, ...), (2) resolve relative
implicit_url against callback_url using urljoin so implicit_url becomes absolute
before posting, and (3) add the required imports from urllib.parse (urljoin,
urlsplit) at top of file; keep the same headers ("Content-Type": "text/plain")
when posting the fragment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f1f394c-2f28-49a6-8adb-5c7f18cbec31
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/conformance/config.template.json.github/conformance/docker-compose.yml.github/conformance/expected-skips.json.github/conformance/run.sh.github/scripts/run-test-plan.py.github/workflows/conformance.yaml.gitignorego.mod
Comment on lines
+39
to
+44
| echo "==> Generating test config..." | ||
| sed \ | ||
| -e "s|\${DISCOVERY_URL}|$DISCOVERY_URL|g" \ | ||
| -e "s|\${CLIENT_ID}|$CLIENT_ID|g" \ | ||
| -e "s|\${CLIENT_SECRET}|$CLIENT_SECRET|g" \ | ||
| "$SCRIPT_DIR/config.template.json" > "$SCRIPT_DIR/results/config.json" |
There was a problem hiding this comment.
sed で JSON テンプレートを埋めるのは壊れやすいです。
Line 40-43 は置換文字列をエスケープしていないので、CLIENT_SECRET / CLIENT_ID / DISCOVERY_URL に & や \ が入ると config.json が壊れます。ここは文字列置換ではなく JSON として代入した方が安全です。
修正例
echo "==> Generating test config..."
-sed \
- -e "s|\${DISCOVERY_URL}|$DISCOVERY_URL|g" \
- -e "s|\${CLIENT_ID}|$CLIENT_ID|g" \
- -e "s|\${CLIENT_SECRET}|$CLIENT_SECRET|g" \
- "$SCRIPT_DIR/config.template.json" > "$SCRIPT_DIR/results/config.json"
+jq \
+ --arg discovery_url "$DISCOVERY_URL" \
+ --arg client_id "$CLIENT_ID" \
+ --arg client_secret "$CLIENT_SECRET" \
+ '.server.discoveryUrl = $discovery_url
+ | .client.client_id = $client_id
+ | .client.client_secret = $client_secret' \
+ "$SCRIPT_DIR/config.template.json" > "$SCRIPT_DIR/results/config.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/conformance/run.sh around lines 39 - 44, The sed-based substitution
(the multi-line sed that replaces ${DISCOVERY_URL}, ${CLIENT_ID},
${CLIENT_SECRET} into config.template.json) is unsafe for values containing &,
\, or other special chars; replace the sed approach with a JSON-aware assignment
using a tool like jq: read the template (config.template.json), set the keys for
DISCOVERY_URL, CLIENT_ID and CLIENT_SECRET from the environment variables
(DISCOVERY_URL, CLIENT_ID, CLIENT_SECRET) with proper JSON escaping, and write
the result to the same output file ("results/config.json")—locate the sed
invocation and the variables DISCOVERY_URL/CLIENT_ID/CLIENT_SECRET in the script
and swap it for a jq-based read/update/write sequence to ensure correct
escaping.
Comment on lines
+110
to
+125
| print(" Browser: following redirect to callback") | ||
| try: | ||
| cb_resp = browser.get(callback_url) | ||
| except httpx.HTTPError as e: | ||
| print(f" Browser: callback request failed: {e}") | ||
| return | ||
|
|
||
| match = re.search(r"xhr\.open\('POST',\s*\"([^\"]+)\"", cb_resp.text) | ||
| if not match: | ||
| print(" Browser: no implicit submit URL found in callback page") | ||
| return | ||
|
|
||
| implicit_url = match.group(1).replace("\\/", "/") | ||
| print(" Browser: submitting fragment to implicit endpoint") | ||
| try: | ||
| browser.post(implicit_url, content="", headers={"Content-Type": "text/plain"}) |
There was a problem hiding this comment.
フラグメントを送っていないため implicit/hybrid の戻りが再現できません。
Line 112 で callback URL を開いた後、Line 125 では implicit endpoint に空文字を POST しています。#fragment は HTTP リクエストに載らないので、ブラウザを模倣するなら callback_url から取り出して明示的に送る必要があります。相対 implicit_url も callback_url 基準で解決した方が安全です。
修正例
- implicit_url = match.group(1).replace("\\/", "/")
+ implicit_url = urljoin(callback_url, match.group(1).replace("\\/", "/"))
+ fragment = urlsplit(callback_url).fragment
print(" Browser: submitting fragment to implicit endpoint")
try:
- browser.post(implicit_url, content="", headers={"Content-Type": "text/plain"})
+ browser.post(
+ implicit_url,
+ content=fragment,
+ headers={"Content-Type": "text/plain"},
+ )
except httpx.HTTPError as e:
print(f" Browser: implicit submit failed: {e}")あわせて import を追加してください。
from urllib.parse import urljoin, urlsplit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/run-test-plan.py around lines 110 - 125, The test runner
opens callback_url then posts an empty body to implicit_url, but fragments (the
`#fragment`) are not sent in HTTP requests so the implicit/hybrid response isn't
reproduced; update the flow in the block handling callback_url/implicit_url so
you (1) parse the fragment out of callback_url (use urlsplit to extract
fragment) and include that fragment as the POST body when calling
browser.post(implicit_url, ...), (2) resolve relative implicit_url against
callback_url using urljoin so implicit_url becomes absolute before posting, and
(3) add the required imports from urllib.parse (urljoin, urlsplit) at top of
file; keep the same headers ("Content-Type": "text/plain") when posting the
fragment.
saitenntaisei
requested changes
Mar 11, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/scripts/run-test-plan.py (1)
227-243: オプション: REVIEW 結果を FAIL と区別して表示することを検討してください。現在の実装では
REVIEWは[FAIL]として表示されますが、手動確認が必要なテストと実際の失敗を区別できると、CI 結果の解釈が容易になります。♻️ REVIEW を分離して表示する例
print("\n=== Summary ===") passed_count = 0 failed_count = 0 + review_count = 0 skipped_count = 0 for r in results: if r["result"] == "SKIPPED": status_mark = "SKIP" skipped_count += 1 elif r["result"] in ("PASSED", "WARNING"): status_mark = "PASS" passed_count += 1 + elif r["result"] == "REVIEW": + status_mark = "REVIEW" + review_count += 1 else: status_mark = "FAIL" failed_count += 1 print(f" [{status_mark}] {r['module']}: {r['result']}") total = len(results) - print(f"\n Total: {total} Passed: {passed_count} Failed: {failed_count} Skipped: {skipped_count}") + print(f"\n Total: {total} Passed: {passed_count} Failed: {failed_count} Review: {review_count} Skipped: {skipped_count}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/run-test-plan.py around lines 227 - 243, Update the loop that summarizes test results to treat "REVIEW" as its own status instead of mapping it to FAIL: add a review_count variable (e.g., review_count = 0), add an elif branch checking if r["result"] == "REVIEW" that sets status_mark = "REVIEW" and increments review_count, ensure "REVIEW" is not counted in failed_count, and update the final print/f-string that prints totals to include review_count in the summary; modify the loop handling around results, passed_count, failed_count, skipped_count, and the final print statement accordingly so the output shows a separate REVIEW column.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/run-test-plan.py:
- Around line 153-158: load_expected_skips currently trusts json.load and the
structure of entries and will crash on invalid JSON or malformed entries; update
load_expected_skips to catch json.JSONDecodeError and OSError when
opening/reading the file, validate that the parsed value is a list, iterate
entries checking each is a dict with a "test_module" key (skip or log malformed
items), and return a set of test_module values while raising or logging a clear,
user-friendly error if the file is not valid JSON or the top-level structure is
not an array.
---
Nitpick comments:
In @.github/scripts/run-test-plan.py:
- Around line 227-243: Update the loop that summarizes test results to treat
"REVIEW" as its own status instead of mapping it to FAIL: add a review_count
variable (e.g., review_count = 0), add an elif branch checking if r["result"] ==
"REVIEW" that sets status_mark = "REVIEW" and increments review_count, ensure
"REVIEW" is not counted in failed_count, and update the final print/f-string
that prints totals to include review_count in the summary; modify the loop
handling around results, passed_count, failed_count, skipped_count, and the
final print statement accordingly so the output shows a separate REVIEW column.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce0780e5-e507-4480-892e-59ff8b4c81b6
📒 Files selected for processing (2)
.github/conformance/run.sh.github/scripts/run-test-plan.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/conformance/run.sh
Comment on lines
+153
to
+158
| def load_expected_skips(path: str | None) -> set[str]: | ||
| if not path or not os.path.exists(path): | ||
| return set() | ||
| with open(path) as f: | ||
| entries = json.load(f) | ||
| return {e["test_module"] for e in entries} |
There was a problem hiding this comment.
JSON パース時のエラーハンドリングが不足しています。
expected-skips.json の形式が不正な場合（JSONパースエラー、test_module キーの欠落、配列でない場合など）、分かりにくいエラーでクラッシュします。
🛡️ エラーハンドリングの追加案
def load_expected_skips(path: str | None) -> set[str]:
if not path or not os.path.exists(path):
return set()
- with open(path) as f:
- entries = json.load(f)
- return {e["test_module"] for e in entries}
+ try:
+ with open(path) as f:
+ entries = json.load(f)
+ if not isinstance(entries, list):
+ print(f"Warning: expected-skips file should contain a JSON array: {path}")
+ return set()
+ return {e["test_module"] for e in entries if "test_module" in e}
+ except (json.JSONDecodeError, KeyError) as e:
+ print(f"Warning: failed to load expected-skips from {path}: {e}")
+ return set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/run-test-plan.py around lines 153 - 158, load_expected_skips
currently trusts json.load and the structure of entries and will crash on
invalid JSON or malformed entries; update load_expected_skips to catch
json.JSONDecodeError and OSError when opening/reading the file, validate that
the parsed value is a list, iterate entries checking each is a dict with a
"test_module" key (skip or log malformed items), and return a set of test_module
values while raising or logging a clear, user-friendly error if the file is not
valid JSON or the top-level structure is not an array.
saitenntaisei
requested changes
Mar 11, 2026
.github/scripts/run-test-plan.py
Outdated
| status_mark = "SKIP" | ||
| skipped_count += 1 | ||
| elif r["result"] in ("PASSED", "WARNING", "REVIEW"): | ||
| elif r["result"] in ("PASSED", "WARNING"): |
Collaborator
There was a problem hiding this comment.
本当に申し訳ないのですが, これREVIEWは人が画像の確認しないといけないのでpassでよいです. REVIEWをPASSED扱いにしていると勘違いしていました。
saitenntaisei
previously approved these changes
Mar 11, 2026
This reverts commit 2e77ea2.
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#57 で conflict などがあったので、とりあえず cherry-pick して衝突のないようにしました
ここから、先の PR で対応しきれなかった部分を追加で編集していきます
Summary by CodeRabbit