Merge pull request #401 from topcoder-platform/PM-4299-1

Tweaks to the artifacts for ECS runner

Author: jmgasper

docs/submission-phase-scoring.md

@@ -101,6 +101,8 @@ Runner task logs are written to CloudWatch using the submission runner log strea
 
 For `PROVISIONAL` and `SYSTEM` scoring, the runner writes progress to the phase review summation metadata through `POST /v6/marathon-match/internal/scoring-progress`. Review API exposes this as `reviewSummation.metadata.testProcess` (`provisional` or `system`), `reviewSummation.metadata.testProgress` (`0` to `1`), and `reviewSummation.metadata.testStatus` (`IN PROGRESS`, `SUCCESS`, or `FAILED`) when metadata is included in the response. In-progress summations keep a neutral placeholder score and must be rendered as unavailable based on `testStatus`; only `FAILED` progress uses the failed-score sentinel. SYSTEM timeout failures also include `reviewSummation.metadata.timed_out = true`.
 
+For generic runner artifacts, the public provisional/system `output.txt` includes testcase ordinal, runtime, runner/tester errors, and submitted-solution stderr, but excludes submitted-solution stdout and raw per-test scores. Per-test score details remain in the private internal `reviews.json` artifact and callback metadata.
+
 When more than one phase config matches the currently open challenge phases, the handler launches one scorer task per match. This is the supported way to run both `EXAMPLE` and `PROVISIONAL` scoring from the same Submission phase.
 
 ## Relative scoring

ecs-runner/README.md

@@ -141,6 +141,8 @@ Once this is saved and the config is active, new scoring launches use the new EC
 
 The runner rejects output once generated public `output.txt` content or a public/private artifact archive would exceed `MM_RUNNER_MAX_OUTPUT_BYTES`. The default is `10000000` bytes. The trusted parent runner passes the resolved limit into the isolated tester child, so generated generic-runner output and uploaded artifact zips use the same cap.
 
+Generic runner public `output.txt` entries show testcase ordinal, runtime, runner/tester errors, and submitted-solution stderr. They intentionally omit submitted-solution stdout and raw per-test scores. The private internal artifact (`reviews.json` in the `*-internal` zip) carries the per-test `testScores` used for diagnostics and score verification.
+
 ## Local smoke test
 
 ```bash

ecs-runner/src/main/java/com/topcoder/runner/EcsRunnerMain.java

@@ -3023,6 +3023,8 @@ private static Path ensurePrivateArtifactsDir(Path submissionDir) throws IOExcep
 
     /**
      * Builds the JSON payload stored in the internal {@code reviews.json} artifact.
+     * The payload includes sanitized top-level {@code testScores} so private
+     * consumers can inspect per-test scores even though public artifacts hide them.
      *
      * @param submissionId Submission whose review payload is archived.
      * @param testPhase Scoring phase represented by the payload.
@@ -3066,12 +3068,42 @@ private static Map<String, Object> buildInternalReviewArtifactPayload(
         payload.put("scorecardId", scorecardId);
         payload.put("score", testerExecution.getScore());
         payload.put("metadata", callbackMetadata);
+        payload.put(
+            "testScores",
+            buildInternalArtifactScoreEntries(testerExecution.getMetadata())
+        );
         payload.put("currentReview", currentReview);
         payload.put("impactedReviews", impactedReviews);
         payload.put("reviews", reviews);
         return payload;
     }
 
+    /**
+     * Extracts per-test score entries for the private review artifact.
+     *
+     * <p>The member-visible public artifact intentionally omits per-test scores,
+     * but internal tooling still needs those values for diagnostics and score
+     * verification. Seed-bearing fields are removed so the private artifact keeps
+     * the same stable testcase identifiers used by callback metadata.
+     *
+     * @param metadata Structured tester metadata produced by generic or custom runner code.
+     * @return Sanitized per-test score entries, or an empty list when none were produced.
+     */
+    private static List<Object> buildInternalArtifactScoreEntries(
+        Map<String, Object> metadata
+    ) {
+        if (metadata == null) {
+            return new ArrayList<Object>();
+        }
+
+        Object testScores = metadata.get("testScores");
+        if (!(testScores instanceof List)) {
+            return new ArrayList<Object>();
+        }
+
+        return sanitizeMemberVisibleScoreEntries((List<?>) testScores);
+    }
+
     /**
      * Adds the runner's callback defaults to a tester-supplied current review payload.
      *
@@ -4148,38 +4180,14 @@ private static TesterExecutionResult runGenericMarathonTester(
                 seedResult.put("error", seedError);
                 testScores.add(seedResult);
 
-                StringBuilder testOutputText = new StringBuilder();
-                testOutputText.append("Test Case #").append(testCaseNumber).append(":\n");
-                testOutputText.append("Score = ").append(seedScore).append('\n');
-                testOutputText.append("Run Time = ")
-                    .append(testResult.getRunTime())
-                    .append("ms\n");
-                if (
-                    testResult.getError() != null
-                        && !testResult.getError().trim().isEmpty()
-                ) {
-                    testOutputText.append(testResult.getError().trim()).append('\n');
-                }
-                if (
-                    testResult.getStdout() != null
-                        && !testResult.getStdout().trim().isEmpty()
-                ) {
-                    testOutputText.append("stdout:\n")
-                        .append(testResult.getStdout().trim())
-                        .append('\n');
-                }
-                if (
-                    testResult.getStderr() != null
-                        && !testResult.getStderr().trim().isEmpty()
-                ) {
-                    testOutputText.append("stderr:\n")
-                        .append(testResult.getStderr().trim())
-                        .append('\n');
-                }
-                testOutputText.append('\n');
                 outputBytes = appendLimitedOutput(
                     outputText,
-                    testOutputText.toString(),
+                    buildMemberVisibleTestOutput(
+                        testCaseNumber,
+                        testResult.getRunTime(),
+                        seedError,
+                        testResult.getStderr()
+                    ),
                     outputBytes,
                     "artifacts/public/output.txt"
                 );
@@ -4241,6 +4249,40 @@ private static TesterExecutionResult runGenericMarathonTester(
         }
     }
 
+    /**
+     * Builds the public {@code output.txt} section for one generic Marathon test case.
+     *
+     * <p>The public provisional artifact shows runtime, tester errors, and stderr
+     * diagnostics, but it intentionally hides submitted-solution stdout and raw
+     * per-test scores. Per-test scores remain available in metadata and the
+     * private {@code reviews.json} artifact.
+     *
+     * @param testCaseNumber Stable 1-based testcase ordinal shown to members.
+     * @param runTimeMs Runtime reported by the Marathon tester for the testcase.
+     * @param error Tester or runner error text for the testcase.
+     * @param stderr Submitted solution stderr captured for the testcase.
+     * @return Member-visible text block ending with a blank line.
+     */
+    private static String buildMemberVisibleTestOutput(
+        int testCaseNumber,
+        long runTimeMs,
+        String error,
+        String stderr
+    ) {
+        StringBuilder testOutputText = new StringBuilder();
+        testOutputText.append("Test Case #").append(testCaseNumber).append(":\n");
+        testOutputText.append("Run Time = ").append(runTimeMs).append("ms\n");
+
+        if (error != null && !error.trim().isEmpty()) {
+            testOutputText.append(error.trim()).append('\n');
+        }
+        if (stderr != null && !stderr.trim().isEmpty()) {
+            testOutputText.append("stderr:\n").append(stderr.trim()).append('\n');
+        }
+        testOutputText.append('\n');
+        return testOutputText.toString();
+    }
+
     /**
      * Defers an isolated child temporary path until the child has emitted its
      * final success or failure output.

ecs-runner/src/test/java/com/topcoder/runner/EcsRunnerMainTest.java

@@ -1,16 +1,23 @@
 package com.topcoder.runner;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.OutputStream;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -49,6 +56,94 @@ public void createPublicArtifactZipRejectsOversizedOutput() throws Exception {
         }
     }
 
+    @Test
+    public void buildMemberVisibleTestOutputHidesStdoutAndScoreButShowsStderr()
+        throws Exception {
+        Method method = EcsRunnerMain.class.getDeclaredMethod(
+            "buildMemberVisibleTestOutput",
+            int.class,
+            long.class,
+            String.class,
+            String.class
+        );
+        method.setAccessible(true);
+
+        String output = (String) method.invoke(
+            null,
+            3,
+            42L,
+            "tester error",
+            "solution stderr"
+        );
+
+        assertTrue(output.contains("Test Case #3:"));
+        assertTrue(output.contains("Run Time = 42ms"));
+        assertTrue(output.contains("tester error"));
+        assertTrue(output.contains("stderr:"));
+        assertTrue(output.contains("solution stderr"));
+        assertFalse(output.contains("Score ="));
+        assertFalse(output.contains("stdout:"));
+    }
+
+    @Test
+    public void buildInternalReviewArtifactPayloadIncludesPerTestScores()
+        throws Exception {
+        Map<String, Object> scoreEntry = new LinkedHashMap<String, Object>();
+        scoreEntry.put("testcase", "1");
+        scoreEntry.put("seed", 12345L);
+        scoreEntry.put("score", 98.5);
+        scoreEntry.put("runTimeMs", 31L);
+
+        List<Map<String, Object>> testScores =
+            new ArrayList<Map<String, Object>>();
+        testScores.add(scoreEntry);
+
+        Map<String, Object> metadata = new LinkedHashMap<String, Object>();
+        metadata.put("testScores", testScores);
+
+        Map<String, Object> currentReview = new LinkedHashMap<String, Object>();
+        currentReview.put("metadata", metadata);
+
+        Object testerExecution = createTesterExecutionResult(
+            98.5,
+            metadata,
+            currentReview
+        );
+        Method method = EcsRunnerMain.class.getDeclaredMethod(
+            "buildInternalReviewArtifactPayload",
+            String.class,
+            String.class,
+            String.class,
+            String.class,
+            getTesterExecutionResultClass(),
+            Map.class
+        );
+        method.setAccessible(true);
+
+        @SuppressWarnings("unchecked")
+        Map<String, Object> payload = (Map<String, Object>) method.invoke(
+            null,
+            "submission-id",
+            "provisional",
+            "review-type-id",
+            "scorecard-id",
+            testerExecution,
+            metadata
+        );
+
+        Object payloadScores = payload.get("testScores");
+        assertTrue(payloadScores instanceof List);
+        List<?> scoreList = (List<?>) payloadScores;
+        assertEquals(1, scoreList.size());
+        assertTrue(scoreList.get(0) instanceof Map);
+
+        Map<?, ?> internalScore = (Map<?, ?>) scoreList.get(0);
+        assertEquals("1", internalScore.get("testcase"));
+        assertEquals(98.5, ((Number) internalScore.get("score")).doubleValue(), 0.0);
+        assertEquals(31L, ((Number) internalScore.get("runTimeMs")).longValue());
+        assertFalse(internalScore.containsKey("seed"));
+    }
+
     private Path createArtifactsDir() throws Exception {
         Path artifactsDir = temporaryFolder.newFolder("artifacts").toPath();
         Files.createDirectories(artifactsDir.resolve("public"));
@@ -72,6 +167,28 @@ private Path invokeCreatePublicArtifactZip(Path artifactsDir) throws Exception {
         );
     }
 
+    private Object createTesterExecutionResult(
+        double score,
+        Map<String, Object> metadata,
+        Map<String, Object> currentReview
+    ) throws Exception {
+        Constructor<?> constructor = getTesterExecutionResultClass()
+            .getDeclaredConstructor(double.class, Map.class, Map.class, List.class);
+        constructor.setAccessible(true);
+        return constructor.newInstance(
+            score,
+            metadata,
+            currentReview,
+            new ArrayList<Map<String, Object>>()
+        );
+    }
+
+    private Class<?> getTesterExecutionResultClass() throws Exception {
+        return Class.forName(
+            "com.topcoder.runner.EcsRunnerMain$TesterExecutionResult"
+        );
+    }
+
     private long getMaxOutputBytes() throws Exception {
         Field field = EcsRunnerMain.class.getDeclaredField("MAX_OUTPUT_BYTES");
         field.setAccessible(true);