Conversation
…-api-v6 into develop
… use in platform-ui
What was broken: Copilot JWTs were rejected by the marathon match scorer/tester setup endpoints used by the Work challenge editor, so the scorer section showed insufficient permissions and MM challenges could not be saved. Root cause: The affected Marathon Match config and tester controller routes only declared @roles(UserRole.Admin), so regular copilot users could not satisfy role-based authorization. What was changed: Added UserRole.Copilot to the scorer/tester setup routes the UI depends on, kept unrelated MM endpoints unchanged, updated the controller/API docs, and documented the new role access in the README/setup guide. Any added/updated tests: Added src/shared/guards/copilot-access.spec.ts to assert the affected setup routes include both Admin and Copilot roles.
What was broken Marathon Match config reads returned the stored reviewScorecardId unchanged. When a config still held a legacy scorecard id, autopilot reused that value while creating SYSTEM reviews in the Review phase, so final-score reviews were not created correctly and final scores never appeared. Root cause The marathon-match config read path did not resolve legacy review scorecard ids to the canonical review-api scorecard primary key, even though downstream review creation expects the canonical id. What was changed Added review-api scorecard resolution with cached lookups to the MM config GET path before returning reviewScorecardId. Added focused warning logs for lookup failures while preserving the existing config response as a fallback. Any added/updated tests Added a unit test covering legacy-to-canonical review scorecard resolution in marathon-match config reads.
What was broken: PUT marathon match config requests accepted any non-empty reviewScorecardId value, so invalid scorecard ids like "x" were persisted successfully. Root cause: The update path only relied on DTO string validation and never verified that reviewScorecardId resolved through review-api. What was changed: Added update-time reviewScorecardId validation in MarathonMatchConfigService using the existing review-api lookup path, returning 400 for unknown scorecards and 500 when validation dependencies are unavailable. The update path now also trims a validated scorecard id before persistence. Updated the setup docs to clarify that the field must resolve through review-api and adjusted the README wording to match the canonical/legacy-id behavior. Any added/updated tests: Added a MarathonMatchConfigService unit test covering PUT/update rejection when reviewScorecardId lookup returns 404.
What was broken:\nPOST /internal/scoring-results accepted scorer callbacks even when challengeId did not belong to any Marathon Match config.\n\nRoot cause:\nScoringResultService.processScoringResult used challengeId only as callback metadata and never verified that a persisted Marathon Match config existed before accepting the request.\n\nWhat was changed:\nAdded a required config lookup at the start of scorer callback processing, reused that resolved config for relative-scoring settings, documented the 404 response on the controller/README, and kept the change scoped to the scoring callback path.\n\nAny added/updated tests:\nAdded ScoringResultService unit coverage for rejecting unknown challengeIds and for continuing to persist review summations when the challenge is configured.
What was broken: POST /internal/scoring-results returned a 500 Internal Server Error when review-api rejected a scoring-result payload that referenced a nonexistent submissionId. Root cause: ScoringResultService wrapped downstream review-api 400/404 responses in a generic Error during review summation writes, so Nest surfaced them as unhandled 500 responses. What was changed: Mapped review-api 400/404 failures from review summation create/update calls to BadRequestException so invalid submission references are returned as client errors instead of internal server errors. Updated the endpoint swagger response description to document the invalid submissionId case. Any added/updated tests: Added a ScoringResultService regression test that mocks review-api rejecting a nonexistent submissionId and verifies the service now throws BadRequestException.
What was broken:
Swagger rendered POST /v6/marathon-match/internal/scoring-results with an empty {} request body schema.
Root cause:
ScoringResultCallbackDto only had validation decorators, so Nest Swagger generated an empty schema for the request body.
What was changed:
Added explicit Swagger property metadata to the scoring result callback DTO so the request body fields are documented.
Added a Swagger regression spec that generates the OpenAPI document and asserts the scoring-results request body schema exposes the expected fields.
Any added/updated tests:
Added src/api/scoring-result/scoring-result.controller.spec.ts for the scoring-results request body schema.
What was broken:\n- GET /testers/:id and PUT /testers/:id returned the compiled tester jar by default, which made responses for compiled testers much slower than necessary.\n\nRoot cause:\n- The tester service fetched jar bytes on normal detail/update reads and converted them to base64 even when callers only needed tester metadata and source.\n\nWhat was changed:\n- Default tester detail/update queries now select source and metadata without jar bytes.\n- Added an explicit includeJarFile query option for GET/PUT responses when the compiled jar is actually needed.\n- Updated README and setup docs to describe the lighter default payload.\n\nAny added/updated tests:\n- Added TesterService coverage for default jar omission, explicit jar inclusion, and update-response behavior.
What was broken: - POST /challenge/:challengeId returned 500 responses for unknown challenge ids and duplicate config creates. - Create requests accepted reviewScorecardId values that review-api could not resolve. Root cause: - createConfig skipped reviewScorecardId validation and relied on generic error wrapping for challenge-api and Prisma failures, which converted upstream 404s and unique constraint violations into 500 responses. What was changed: - Validated challenge ids during create and reused the same challenge-api validation in phase normalization. - Validated reviewScorecardId during create. - Translated duplicate challenge config inserts into 409 Conflict and updated create endpoint/setup docs. Any added/updated tests: - Added create-path unit tests for invalid challenge ids, invalid review scorecard ids, and duplicate config conflicts in marathon-match-config.service.spec.ts.
What was broken: POST /testers and PUT /testers/:id accepted whitespace-only values for name, version, sourceCode, and className. Root cause: The tester DTOs only used IsNotEmpty, which rejects empty strings but still allows strings containing only whitespace. What was changed: Added non-whitespace validation to the tester create/update DTO fields and updated the field docs to describe the constraint. Added a focused validation spec that covers create/update rejection while still allowing indented source code with real content. Any added/updated tests: Added src/dto/tester.dto.spec.ts for whitespace-only POST/PUT payload validation.
What was broken PUT /testers/:id overwrote the existing tester row, so older tester versions disappeared and the work-app scorer editor could not reliably keep version history available for lookup and selection. The API also accepted non-incrementing tester versions because it never compared the submitted version against the current max version for that tester family. Root cause The original PM-4297 implementation added tester management endpoints, but the follow-up version flow was still implemented as an in-place update instead of creating a new tester-version record. There was no server-side version comparison enforcing the UI rule that each new tester build must be higher than the current max version. What was changed Changed PUT /testers/:id to create a new tester version record using the referenced tester family name, while preserving older versions for future lookup and selection. Added numeric-aware tester version comparison and now reject submitted versions that are not higher than the current max version for that tester family. Updated the tester DTO/controller docs and setup docs to reflect the preserved-version workflow and the returned tester ID that should be polled after version creation. Any added/updated tests Added TesterService coverage for successful tester-version creation and for rejecting non-incrementing versions. Updated tester DTO validation coverage for the new version DTO and adjusted the copilot-access metadata spec for the renamed controller method.
PM-4297: preserve tester versions for new scorer builds
What was broken POST /testers could still create another record for an existing tester name, so the work-app could bypass the PUT versioning flow and mix duplicate tester families into the name-grouped lookup. Root cause The prior PM-4297 fix corrected PUT /testers/:id, but POST /testers never enforced that a tester name is only created once as the family seed. What was changed POST /testers now checks for an existing tester family name and returns 409 Conflict with guidance to use PUT /testers/:id for higher versions. Updated the tester create DTO/controller docs plus the setup docs to clarify that POST only creates the first version of a tester family. Any added/updated tests Added TesterService coverage for rejecting duplicate tester-family creation through POST while leaving the existing PUT versioning coverage in place.
PM-4297: block duplicate tester-family creation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Initial production release for marathon-match scoring, expanding admin/copilot setup flows and operational tooling around ECS scoring tasks.
Changes:
- Adds new scorer callback + runner log retrieval APIs and persists ECS task/log mappings to Postgres.
- Refactors tester/version management (DTOs, controller/service behavior) and improves compilation orchestration (pg-boss toggle, temp dir handling, Maven opts).
- Updates Prisma/Postgres schema & migrations (new config fields, new tables/enums) and introduces a hardened ECS runner image + boilerplate.
Reviewed changes
Copilot reviewed 75 out of 92 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/modules/kafka/handlers/marathon-match-submission.handler.ts | Expands Kafka message typing to support event-bus envelopes. |
| src/shared/modules/global/prisma.service.ts | Switches Prisma to adapter-pg and adds datasource/schema resolution helpers. |
| src/shared/modules/global/ecs.service.ts | Adds richer ECS launch result, persists submission→log mapping, passes phase env. |
| src/shared/guards/copilot-access.spec.ts | Adds source-level test to enforce copilot role access on setup routes. |
| src/shared/config/m2m.config.ts | Renames M2M/Auth0 env var keys used for token acquisition. |
| src/dto/tester.dto.ts | Splits tester DTOs, hardens validation, adds includeJarFile query DTO, adds summary DTO. |
| src/dto/tester.dto.spec.ts | Adds validation tests for whitespace-only fields. |
| src/dto/marathon-match-config.dto.ts | Adds relative scoring fields and new defaults/rerun response DTOs. |
| src/api/tester/tester.service.ts | Introduces tester families/version-create semantics; adds jar gating; improves list selects. |
| src/api/tester/tester.service.spec.ts | Adds unit tests for jar gating, version constraints, and conflict behavior. |
| src/api/tester/tester.controller.ts | Updates endpoints/roles/docs; turns PUT into “create version”; adds includeJarFile query. |
| src/api/tester/tester-compilation.service.ts | Adds inline compilation mode, temp dir probing, Maven/JVM tmp controls and heap tuning. |
| src/api/tester/compilation-worker.service.ts | Makes pg-boss queue name configurable-ish and supports disabling worker startup. |
| src/api/submission-runner-log/submission-runner-log.service.ts | Implements CloudWatch log retrieval using persisted runner log mappings. |
| src/api/submission-runner-log/submission-runner-log.controller.ts | Adds secured endpoint to fetch runner logs by submissionId with pagination params. |
| src/api/scoring-result/scoring-result.service.spec.ts | Adds service tests for config lookup and error mapping behavior. |
| src/api/scoring-result/scoring-result.controller.ts | Adds internal callback + system-score endpoints with Swagger models. |
| src/api/scoring-result/scoring-result.controller.spec.ts | Adds Swagger schema coverage test for the callback DTO. |
| src/api/marathon-match-config/marathon-match-config.service.spec.ts | Adds tests around scorecard resolution and create/update error cases. |
| src/api/marathon-match-config/marathon-match-config.controller.ts | Adds defaults + rerun endpoints and broadens roles to include copilot for setup. |
| src/api/kafka/marathon-match-submission.handler.spec.ts | Adds test coverage for multi-phase dispatch behavior. |
| src/api/api.module.ts | Registers new controllers/services (scoring result + runner logs). |
| prisma/schema.prisma | Adds ScoreDirection enum, new submissionRunnerLog model, and config schema changes. |
| prisma/migrations/20260309093000_add_relative_scoring_config/migration.sql | Migration for relative scoring fields + enum. |
| prisma/migrations/20260307090000_add_submission_runner_log_mapping/migration.sql | Migration for submissionRunnerLog table + indexes. |
| prisma/migrations/20260305121500_split_marathon_config_id_from_challenge_id/migration.sql | Migration to decouple config id from challengeId and retrofit nanoid PKs + FK updates. |
| prisma/migrate.ts | Updates migrate bootstrap to use adapter-pg and configurable schema creation. |
| prisma.config.ts | Adds Prisma CLI config with env loading + datasource url wiring. |
| package.json | Adds CloudWatch logs client, pg + prisma adapter, and pins prisma/client versions. |
| ecs-runner/scripts/mm-net-isolate.c | Adds seccomp-based socket restrictions + env scrubbing helper for isolated execution. |
| ecs-runner/scripts/entrypoint.sh | Enforces root start and makes JVM options configurable. |
| ecs-runner/scripts/build-and-push-ecr.sh | Adds helper script to buildx/push runner image to ECR. |
| ecs-runner/pom.xml | Adds httpmime dependency. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/services/SubmissionService.java | Adds submission-api download/unzip support with logs. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/services/ReviewService.java | Adds submission-api review posting helper (currently very verbose). |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/services/ParameterStoreService.java | Adds placeholder file (currently empty). |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/models/ScoringResult.java | Adds scorer result model. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/models/ScorerConfig.java | Adds scorer config model. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/models/PhaseConfig.java | Adds phase config model. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/models/ChallengeConfig.java | Adds challenge config model. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/scorer/ScorerMain.java | Adds Java scorer main orchestrator (currently logs env + token previews). |
| ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/Parameters.java | Ports marathon tester parameter parsing/utilities. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/MarathonTester.java | Ports marathon tester runtime base class. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/MarathonTestResult.java | Adds test result container. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/MarathonController.java | Ports marathon controller and adds classloader-aware tester loading. |
| ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/ErrorReader.java | Adds async stderr reader for solution processes. |
| ecs-runner/boilerplate/pom.xml | Adds boilerplate Maven project for building tester harness jars. |
| ecs-runner/boilerplate/checker/solution_python.sh | Adds Python compile/run command writer. |
| ecs-runner/boilerplate/checker/solution_java.sh | Adds Java compile/run command writer. |
| ecs-runner/boilerplate/checker/solution_csharp_net7.sh | Adds .NET 7 publish/run command writer. |
| ecs-runner/boilerplate/checker/solution_csharp_mono.sh | Adds Mono compile/run command writer. |
| ecs-runner/boilerplate/checker/solution_cpp.sh | Adds C++ compile/run command writer. |
| ecs-runner/boilerplate/checker/solution.sh | Adds language selection wrapper and compile log redirect. |
| ecs-runner/boilerplate/README.md | Documents boilerplate composition and how compilation worker uses it. |
| ecs-runner/README.md | Documents runner image, isolation model, and operational steps. |
| ecs-runner/Dockerfile | Rebuilds runner image with tools + net isolation helper. |
| docs/submission-phase-scoring.md | Documents submission-phase scoring flow, retries, and observability. |
| docs/review-phase-scoring.md | Documents review-phase orchestration and failure handling. |
| docs/marathon-match-setup.md | Documents end-to-end setup steps including tester versioning and rerun. |
| docker-compose.yml | Updates Kafka env var name used in compose. |
| appStartUp.sh | Updates prisma commands and changes production start command. |
| SetupExample.md | Adds a concrete dev setup/runbook for BridgeRunners example. |
| Dockerfile | Adds Maven + JDK to API image and sets NODE_ENV=production. |
| .env.sample | Updates env var keys, adds defaults/tuning flags and debug runner logging flags. |
| .env.example | Updates env var keys, adds defaults/tuning flags and debug runner logging flags. |
| .circleci/config.yml | Adds CircleCI build+deploy pipeline configuration. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Start the app | ||
| pnpm start:prod | ||
| exec node dist/src/main |
There was a problem hiding this comment.
node dist/src/main will usually fail because TypeScript builds typically emit .js files (for example dist/src/main.js or dist/main.js). This change is likely to break production startup; update the command to point at the actual emitted JS entry file used by your build output.
Suggested change
| exec node dist/src/main | |
| exec node dist/src/main.js |
Comment on lines
+27
to
+35
| // Log all environment variables for debugging | ||
| System.out.println("[DEBUG] Environment Variables:"); | ||
| System.getenv().forEach((k, v) -> { | ||
| if (k.toLowerCase().contains("token")) { | ||
| System.out.println(" " + k + "=***" + (v.length() > 8 ? v.substring(0, 8) : v) + "... (redacted) ***"); | ||
| } else { | ||
| System.out.println(" " + k + "=" + v); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dumping all environment variables is a high-risk secret leak in production (AWS credentials, DB URLs, internal endpoints, etc.), and only redacting keys containing token is insufficient. Gate this behind an explicit debug flag (and default it off), and/or log only a narrow allowlist of non-sensitive keys.
Comment on lines
+116
to
+123
| const runnerEnvironment = [ | ||
| { name: 'TESTER_CONFIG_ID', value: challengeId }, | ||
| { name: 'SUBMISSION_ID', value: submissionId }, | ||
| { name: 'ACCESS_TOKEN', value: token }, | ||
| { name: 'MARATHON_MATCH_API_URL', value: marathonMatchApiUrl }, | ||
| { name: 'REVIEW_TYPE_ID', value: reviewTypeId }, | ||
| { name: 'TEST_PHASE', value: testPhase }, | ||
| { name: 'PHASE_CONFIG_TYPE', value: scoringPhase.configType }, |
There was a problem hiding this comment.
TESTER_CONFIG_ID is being populated with challengeId, which is semantically confusing and easy to misuse in the runner/container. Prefer renaming the env var to something accurate (for example CHALLENGE_ID) or updating the runner contract/docs so the meaning is explicit and consistent.
kkartunov
approved these changes
Apr 14, 2026
… ECS logs permissions
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.
Initial prod release