Skip to content

Commit 7937aff

Browse files
committed
refactor(container): one scorecard source, and a thin session route
Two scorecard extractors coexisted and both ran inside finalize_session: a validated JSON parser feeding cs.scorecard, and an unvalidated regex over the markdown feeding the PR comment. Same session, two formats, two failure modes — a small wording drift in a skill silently produced no comment while the dashboard scored it fine, and pr_comment.py ran its own SQL over the events table to do it, in a module whose sibling repository claims to own every query. Skills deliberately emit both formats: markdown for the human watching the stream, JSON for anything that reads the outcome. Only the JSON is parsed now. The comment is rendered from the same validated object the dashboard stores, so the two cannot disagree about a session. The scorecard is a Pydantic model rather than a dict with hand-rolled checks, which also replaces `REQUIRED_FIELDS`/`len(dims) != 3`/`0 <= v <= 10` with declarations. `extra="allow"`: the declared fields are the contract, not the ceiling. Behaviour change worth knowing: the comment's headline score is now the mean of the reported dimensions instead of the free-text "Score: 8" the skill wrote next to them — a number nothing stopped from disagreeing with the dimensions on the line below it. xp_earned is finally written, from that same scorecard. It was declared on the model, created by a migration, exposed in two response schemas and read by the frontend, and never once assigned. create_container_session goes from a 45-line handler to 27 lines around one call. Authorization, Fernet decryption and token minting were transport- layer work that the webhook path could never reuse; they are `open_session` in the service now.
1 parent 1539806 commit 7937aff

8 files changed

Lines changed: 324 additions & 347 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ Key additions for production: `ENVIRONMENT=production`, `ADMIN_PASSWORD`, `CORS_
143143
- **BYOK via dashboard**: Claude OAuth tokens (from `claude setup-token`) stored per installation, encrypted with Fernet, injected as `CLAUDE_CODE_OAUTH_TOKEN` env var into containers. Zero API cost (uses user's Claude subscription).
144144
- **Dashboard over SQLAdmin**: user-facing operations (installation list, session history, token config) go through the dashboard UI; SQLAdmin is the superadmin escape hatch
145145
- **Open source target**: designed for self-hosting with own Claude licenses
146-
- **Post-results to PR**: after session completion, the API can post score card as a PR comment — opt-in per installation via `post_results_to_pr` boolean; extraction and formatting in `container/pr_comment.py`, triggered in `_event_stream()` after `mark_completed()`
146+
- **Post-results to PR**: after session completion the API can post the scorecard as a PR comment — opt-in per installation via `post_results_to_pr`. Rendered from the validated `Scorecard` model (`container/scorecard.py`), the same object stored on `cs.scorecard` and used for `xp_earned`, so the dashboard and the comment cannot disagree. Skills emit two things: markdown for the human watching the stream, and the `helprs-scorecard` JSON block for everything machine-read. Only the JSON is parsed; `pr_comment.py` formats, it no longer extracts.
147147
- **Coolify deployment**: Two-domain setup via Traefik: `helprs.tech` (web) and `api.helprs.tech` (API). TLS via Let's Encrypt, managed by Coolify. Domains are set in Coolify UI (General > Domains for api / Domains for web) — they may get cleared on redeploy, re-check after each deploy. "Preserve Repository During Deployment" must be enabled so skills are available on the host. The prod compose uses `./` paths (repo-root-relative) because Coolify sets `--project-directory` to the repo root.
148148
- **claude-runner image**: declared in both `docker-compose.yml` and `infra/coolify/docker-compose.prod.yml` as a **build-only service**`entrypoint: ["/bin/true"]` + `restart: "no"` so the container exits immediately on `up`, leaving only the built image `claude-runner:latest` on the host. The API spawns containers from this image dynamically via the mounted Docker socket. Image tag is hard-coded in `container/service.py:CLAUDE_RUNNER_IMAGE` — it must stay `claude-runner:latest` (no namespace prefix). Earlier attempt with `profiles: [build-only]` was reverted: `profiles:` excludes a service from both build AND run unless the profile is explicitly activated, which Coolify does not do, causing `404 No such image` failures on session spawn.
149149
- **Non-root API container**: production Dockerfile uses `appuser` with `chown -R appuser:appuser /app` for uv cache writes. Docker socket access requires `group_add: ["${DOCKER_GID:-994}"]` in the compose to match the host's docker group GID.

apps/api/src/helprs/modules/container/pr_comment.py

Lines changed: 46 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,68 +1,62 @@
1-
"""Score card extraction and PR comment formatting for challenge-me sessions."""
1+
"""Rendering a finished session as a GitHub PR comment.
22
3-
import re
4-
from uuid import UUID
3+
Built from the validated ``Scorecard``, not from the markdown the skill
4+
writes for the live stream. Two extractors used to coexist -- a validated
5+
JSON parser feeding the dashboard and an unvalidated regex over the markdown
6+
feeding this comment -- so one session could be scored in one place and
7+
silently uncommented in the other. There is one machine-readable source now,
8+
and this module no longer runs its own SQL over the events table.
9+
"""
510

6-
import structlog
7-
from sqlalchemy import select
8-
from sqlalchemy.ext.asyncio import AsyncSession
11+
from uuid import UUID
912

10-
from helprs.modules.container.models import SessionEvent
13+
from helprs.modules.container.scorecard import MAX_DIMENSION_SCORE, Scorecard
1114

12-
logger = structlog.get_logger()
15+
_BAR_WIDTH = 10
1316

14-
_RESULTS_PATTERN = re.compile(
15-
r"---\s*\n\s*\n##\s+Results\b(.*?)^---\s*$",
16-
re.DOTALL | re.MULTILINE,
17-
)
1817

18+
def _score_bar(score: float) -> str:
19+
filled = round(score * _BAR_WIDTH / MAX_DIMENSION_SCORE)
20+
return "█" * filled + "░" * (_BAR_WIDTH - filled)
1921

20-
async def extract_score_card(session_id: UUID, db: AsyncSession) -> str | None:
21-
"""Extract the score card markdown from persisted session events.
2222

23-
Queries the last ``result`` event for the session and extracts the
24-
``## Results`` section delimited by ``---`` markers (as defined in
25-
``skills/challenge-me/CLAUDE.md``).
26-
"""
27-
result = await db.execute(
28-
select(SessionEvent.data)
29-
.where(
30-
SessionEvent.session_id == session_id,
31-
SessionEvent.data["type"].astext == "result",
32-
)
33-
.order_by(SessionEvent.event_id.desc())
34-
.limit(1)
23+
def _dimensions_table(scorecard: Scorecard) -> str:
24+
rows = "\n".join(
25+
f"| {name.replace('_', ' ').title()} | {score:g} / {MAX_DIMENSION_SCORE} |"
26+
for name, score in scorecard.dimensions.items()
3527
)
36-
row = result.scalar_one_or_none()
37-
if row is None:
38-
return None
39-
40-
result_text = row.get("result")
41-
if not result_text or not isinstance(result_text, str):
42-
return None
28+
return f"| Dimension | Score |\n|-----------|-------|\n{rows}"
4329

44-
match = _RESULTS_PATTERN.search(result_text)
45-
if not match:
46-
return None
4730

48-
return f"## Results{match.group(1).rstrip()}"
31+
def format_pr_comment(scorecard: Scorecard, session_url: str) -> str:
32+
"""Render the scorecard as the comment body."""
33+
overall = scorecard.overall_score
34+
sections = [
35+
"### helPRs Challenge-Me Results",
36+
"",
37+
f"**Score: {overall:.1f} / {MAX_DIMENSION_SCORE}** {_score_bar(overall)}",
38+
"",
39+
_dimensions_table(scorecard),
40+
"",
41+
scorecard.summary,
42+
]
4943

44+
if scorecard.highlights:
45+
sections += ["", "**Highlights**", *(f"- {item}" for item in scorecard.highlights)]
5046

51-
def format_pr_comment(
52-
score_card: str,
53-
session_url: str,
54-
) -> str:
55-
"""Format the score card as a GitHub PR comment."""
56-
return (
57-
f"### helPRs Challenge-Me Results\n\n"
58-
f"{score_card}\n\n"
59-
f"<details>\n"
60-
f"<summary>View session</summary>\n\n"
61-
f"[Open full Q&A session]({session_url})\n\n"
62-
f"---\n"
63-
f"*Posted by [helPRs](https://github.com/apps/helprs)*\n"
64-
f"</details>\n"
65-
)
47+
sections += [
48+
"",
49+
"<details>",
50+
"<summary>View session</summary>",
51+
"",
52+
f"[Open full Q&A session]({session_url})",
53+
"",
54+
"---",
55+
"*Posted by [helPRs](https://github.com/apps/helprs)*",
56+
"</details>",
57+
"",
58+
]
59+
return "\n".join(sections)
6660

6761

6862
def build_session_url(

apps/api/src/helprs/modules/container/router.py

Lines changed: 11 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,8 @@
1414

1515
from helprs.core.database import get_db_context
1616
from helprs.core.dependencies import CurrentUser, DbSession, GetSettings, authenticate_token, stream_token
17-
from helprs.core.exceptions import ConflictError, NotFoundError
17+
from helprs.core.exceptions import ConflictError
1818
from helprs.core.middleware import limiter
19-
from helprs.core.security import fernet_decrypt
2019
from helprs.modules.container.docker_client import AioDockerClient, DockerClient
2120
from helprs.modules.container.models import ContainerStatus
2221
from helprs.modules.container.schemas import (
@@ -30,23 +29,16 @@
3029
StopSessionResponse,
3130
)
3231
from helprs.modules.container.service import (
33-
create_session,
3432
delete_session,
3533
finalize_session,
3634
get_session_events,
3735
get_session_or_404,
36+
open_session,
3837
send_message,
39-
start_container,
4038
stop_container,
4139
)
4240
from helprs.modules.container.streaming import spawn_detached, stream_and_persist
43-
from helprs.modules.installation.github import RUNNER_TOKEN_PERMISSIONS
4441
from helprs.modules.installation.service import (
45-
get_byok_config,
46-
get_installation_by_github_id,
47-
mint_installation_token,
48-
verify_installation_access,
49-
verify_repo_access,
5042
verify_session_access,
5143
)
5244

@@ -70,53 +62,17 @@ async def create_container_session(
7062
user: CurrentUser,
7163
) -> ContainerSessionResponse:
7264
"""Create a session and start its container."""
73-
installation = await get_installation_by_github_id(db, body.installation_id)
74-
if not installation:
75-
raise NotFoundError("Installation not found")
76-
77-
# Installation membership is not enough: an installation can cover repos
78-
# this user cannot read, and the session streams the diff back to them.
79-
await verify_installation_access(user, installation, settings)
80-
await verify_repo_access(user, body.repo_full_name, settings)
81-
82-
byok_config = await get_byok_config(db, installation.id)
83-
if not byok_config:
84-
raise NotFoundError("No Claude token configured for this installation")
85-
86-
claude_oauth_token = fernet_decrypt(byok_config.encrypted_api_key, settings.fernet_keys)
87-
88-
# Narrowed to this repo, read-only: the container runs Claude Code over
89-
# untrusted PR content with network egress.
90-
github_token = await mint_installation_token(
91-
installation.github_installation_id,
92-
settings,
93-
repositories=[body.repo_full_name.split("/")[-1]],
94-
permissions=RUNNER_TOKEN_PERMISSIONS,
95-
)
96-
97-
cs = await create_session(
98-
db=db,
99-
installation_id=installation.id,
100-
pr_number=body.pr_number,
101-
repo_full_name=body.repo_full_name,
102-
skill_name=body.skill_name,
103-
user_id=user.id,
104-
)
105-
# Committed before the container is touched. start_container marks the row
106-
# FAILED on error and then raises, but that exception unwinds through
107-
# get_db, which rolls back -- discarding the FAILED status *and* the row
108-
# itself. A failed start left the user with a 502 and a dashboard showing
109-
# that nothing had ever happened.
110-
await db.commit()
111-
11265
docker = _get_docker_client()
11366
try:
114-
cs = await start_container(
115-
db=db,
116-
session_id=cs.id,
117-
docker=docker,
118-
claude_oauth_token=claude_oauth_token,
119-
github_token=github_token,
67+
cs = await open_session(
68+
db,
69+
docker,
70+
user=user,
71+
installation_github_id=body.installation_id,
72+
pr_number=body.pr_number,
73+
repo_full_name=body.repo_full_name,
74+
skill_name=body.skill_name,
75+
settings=settings,
12076
)
12177
finally:
12278
await docker.close()
Lines changed: 53 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,74 @@
1-
"""Scorecard extraction from Claude session output.
1+
"""The structured scorecard a skill emits at the end of a session.
22
3-
Skills emit a structured JSON scorecard wrapped in a fenced code block
4-
tagged `helprs-scorecard`. This module parses it from the last assistant
5-
message text.
3+
Skills write two things when they finish: markdown results for the human
4+
watching the stream, and this JSON block for anything that has to *read* the
5+
outcome. Both are documented in ``skills/challenge-me/CLAUDE.md``.
6+
7+
Everything machine-consumed goes through the model below, so the dashboard
8+
and the PR comment can never disagree about a session. The PR comment used to
9+
be regex-scraped out of the markdown instead, with no validation, so a small
10+
wording drift in a skill silently produced no comment at all.
611
"""
712

813
import json
914
import re
1015

16+
from pydantic import BaseModel, ConfigDict, Field, ValidationError, field_validator
17+
1118
SCORECARD_PATTERN = re.compile(
1219
r"```helprs-scorecard\s*\n(.*?)\n```",
1320
re.DOTALL,
1421
)
1522

16-
REQUIRED_FIELDS = {"skill", "version", "dimensions", "summary"}
23+
DIMENSION_COUNT = 3
24+
MAX_DIMENSION_SCORE = 10
1725

1826

19-
def extract_scorecard(text: str) -> dict | None:
20-
"""Extract the helprs-scorecard JSON block from text.
27+
class Scorecard(BaseModel):
28+
"""A skill's verdict on one session.
2129
22-
Returns the parsed dict if found and valid, None otherwise.
30+
``extra="allow"`` because a skill may report more than helPRs reads: the
31+
fields below are the contract, not the ceiling.
2332
"""
24-
match = SCORECARD_PATTERN.search(text)
25-
if not match:
26-
return None
2733

28-
try:
29-
data = json.loads(match.group(1))
30-
except (json.JSONDecodeError, ValueError):
31-
return None
34+
model_config = ConfigDict(extra="allow")
3235

33-
if not isinstance(data, dict):
34-
return None
36+
skill: str
37+
version: int
38+
dimensions: dict[str, float]
39+
summary: str
40+
questions_asked: int | None = None
41+
questions_answered: int | None = None
42+
highlights: list[str] = Field(default_factory=list)
3543

36-
if not REQUIRED_FIELDS.issubset(data.keys()):
37-
return None
44+
@field_validator("dimensions")
45+
@classmethod
46+
def check_dimensions(cls, value: dict[str, float]) -> dict[str, float]:
47+
if len(value) != DIMENSION_COUNT:
48+
raise ValueError(f"expected exactly {DIMENSION_COUNT} dimensions, got {len(value)}")
49+
for name, score in value.items():
50+
if not 0 <= score <= MAX_DIMENSION_SCORE:
51+
raise ValueError(f"dimension '{name}' is {score}, outside 0-{MAX_DIMENSION_SCORE}")
52+
return value
53+
54+
@property
55+
def overall_score(self) -> float:
56+
"""Mean of the dimensions, on the same 0-10 scale."""
57+
return sum(self.dimensions.values()) / len(self.dimensions)
3858

39-
dims = data.get("dimensions")
40-
if not isinstance(dims, dict) or len(dims) != 3:
41-
return None
4259

43-
for value in dims.values():
44-
if not isinstance(value, (int, float)) or value < 0 or value > 10:
45-
return None
60+
def extract_scorecard(text: str) -> Scorecard | None:
61+
"""Parse the ``helprs-scorecard`` block out of text.
4662
47-
return data
63+
Returns ``None`` rather than raising: a session whose skill emitted no
64+
scorecard, or emitted a malformed one, is a session without a scorecard --
65+
not a failed session.
66+
"""
67+
match = SCORECARD_PATTERN.search(text)
68+
if not match:
69+
return None
70+
71+
try:
72+
return Scorecard.model_validate(json.loads(match.group(1)))
73+
except (json.JSONDecodeError, ValueError, ValidationError):
74+
return None

0 commit comments

Comments
 (0)