refactor(container): one scorecard source, and a thin session route - #71
Merged
Conversation
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.
|
helPRs session created for this PR. Skill: |
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.
The two architecture items deferred from #68, plus the
xp_earnedcolumn that was dead since it was created.Two extractors for one concept
Both ran inside
finalize_sessionon every completed session:scorecard.extract_scorecardhelprs-scorecardJSON blockcs.scorecard(dashboard)pr_comment.extract_score_card## Resultsmarkdown, by regexSame session, two formats, two failure modes. A small wording drift in a skill's markdown produced no PR comment at all while the dashboard scored the session fine — silently, since a missing match is indistinguishable from "no scorecard". And
pr_comment.pyran its ownselect(SessionEvent.data)with a JSONB predicate to get there, inside a module whose siblingrepository.pyopens with "every query against them lives here".Skills emit both formats deliberately —
skills/challenge-me/CLAUDE.mdsays "You MUST emit BOTH", markdown for the human watching the stream and JSON for anything that reads the outcome. So the fix is not to make skills emit less; it is to stop parsing the human-facing half. Only the JSON is parsed now, and the comment is rendered from the same validated object the dashboard stores.The scorecard is a model
REQUIRED_FIELDS.issubset(...),len(dims) != 3,0 <= value <= 10were hand-rolled validation. That is a Pydantic model:extra="allow"because the declared fields are the contract, not the ceiling — a skill reporting more should not be rejected.Behaviour change worth flagging
The comment's headline score is now the mean of the reported dimensions rather than the free-text
### Score: 8 / 10the skill wrote next to them. With the fixture's dimensions of 8/7/6 the comment says 7.0, where it previously echoed the skill's "8".That is the point: nothing stopped the prose score from disagreeing with the dimensions on the line below it, and one of the two had to win. The computed one is the one that is checked.
xp_earned
Declared on the model, created by migration
a2b3c4d5e6f8, exposed in two response schemas, consumed bycontainerApi.ts— and never assigned anywhere insrc/. The API returnednullevery time. It is now the mean of the dimensions on a 0-100 scale, written alongside the scorecard, with an end-to-end test asserting it.The route is thin again
create_container_sessionwas 45 lines that resolved an installation, ran two authorization checks, fetched a BYOK config, Fernet-decrypted a credential, minted a scoped GitHub token, created a row, committed, started a container and closed the client. Credential decryption is not transport work, and the webhook path could never reuse any of it.It is
service.open_sessionnow; the handler is 27 lines around one call.Verification
ruff+ruff format+mypycleantest_pr_comment.pyrewritten against the new shape, including the validation gate that decides whether a comment is posted at all