fix(server): compose the feedback a review measured

.changeset/compose-feedback-for-real-reviews.md

@@ -0,0 +1,7 @@
+---
+"hephaestus": minor
+---
+
+Practice reviews deliver feedback again. A review measured a pull request, recorded what it found, and then stopped: the step that turns those findings into something a developer reads was never switched on, so every review ended with its results stored and nothing said. Reviews now compose feedback for each lane the occasion can reach — the note on the work, the developer's own practice pages, and an ongoing conversation — and issue reviews compose for the two longitudinal lanes, since an issue is not the work a note belongs on.
+
+**Operators:** feedback now appears where it previously did not, so a workspace with review switched on begins posting again. Nothing new is required of you, and the instance-wide Silent Mode brake still holds everything back while it is engaged — but if you upgraded during the window where reviews were silent, this is the change that ends it.

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/IssueReviewHandler.java

@@ -10,6 +10,7 @@
 import de.tum.cit.aet.hephaestus.agent.context.PreparedEvidence;
 import de.tum.cit.aet.hephaestus.agent.context.WorkspaceContextBuilder;
 import de.tum.cit.aet.hephaestus.agent.handler.composition.ComposedFeedbackUnit;
+import de.tum.cit.aet.hephaestus.agent.handler.composition.FeedbackCompositionInputs;
 import de.tum.cit.aet.hephaestus.agent.handler.composition.FeedbackCompositionResultParser;
 import de.tum.cit.aet.hephaestus.agent.handler.spi.ExistingDeliveryLookup;
 import de.tum.cit.aet.hephaestus.agent.handler.spi.JobDeliveryException;
@@ -191,7 +192,13 @@ public PreparedJobInputs prepareInputs(AgentJob job) {
         files.put(SandboxLayout.TASK_ENVELOPE_FILENAME, taskEnvelopeWriter.write(buildTaskEnvelope(job, metadata)));
         practiceCatalogInjector.inject(files, job, ArtifactKinds.ISSUE, practices);
         // See PullRequestReviewHandler: a second, separate turn composes this developer's feedback once
-        // the measurements are final — here for the two longitudinal lanes only, per ISSUE_REVIEW_CHANNELS.
+        // the measurements are final. An issue has no diff, so the note it may place is artifact-level.
+        FeedbackCompositionInputs.stage(
+            files,
+            PracticeDetectionDeliveryService.originOf(metadata),
+            ISSUE_REVIEW_CHANNELS,
+            EnumSet.of(FeedbackCompositionInputs.InContextPlacementKind.ARTIFACT)
+        );
         log.info(
             "Issue context preparation complete: {} files, issueNumber={}, jobId={}",
             files.size(),

server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PullRequestReviewHandler.java

@@ -12,6 +12,7 @@
 import de.tum.cit.aet.hephaestus.agent.context.PreparedEvidence;
 import de.tum.cit.aet.hephaestus.agent.context.WorkspaceContextBuilder;
 import de.tum.cit.aet.hephaestus.agent.handler.composition.ComposedFeedbackUnit;
+import de.tum.cit.aet.hephaestus.agent.handler.composition.FeedbackCompositionInputs;
 import de.tum.cit.aet.hephaestus.agent.handler.composition.FeedbackCompositionResultParser;
 import de.tum.cit.aet.hephaestus.agent.handler.spi.ExistingDeliveryLookup;
 import de.tum.cit.aet.hephaestus.agent.handler.spi.JobDeliveryException;
@@ -254,6 +255,7 @@ public PreparedJobInputs prepareInputs(AgentJob job) {
         // Asks the run for a second, separate turn once its measurements are final: the feedback to say
         // now, on every lane this occasion can reach, composed over this person's record rather than over
         // this diff alone. Absent for a backfill sweep — see FeedbackCompositionInputs.
+        FeedbackCompositionInputs.stage(files, PracticeDetectionDeliveryService.originOf(metadata));
         ContextMapWriter.write(files);
 
         long elapsedMs = (System.nanoTime() - startNanos) / 1_000_000;

server/src/main/resources/agent/pi-runner.ts

@@ -162,8 +162,10 @@ function isAdmittedObservation(value: unknown): value is AdmittedObservation {
 	);
 }
 
-const OUTPUT = "/workspace/out";
-const CWD = "/workspace";
+// Overridable so a harness with no /workspace can drive the runner. Production never sets it.
+const WORKSPACE_ROOT = "/workspace";
+const CWD = process.env.PI_RUNNER_CWD ?? WORKSPACE_ROOT;
+const OUTPUT = `${CWD}/out`;
 const RESULT_PATH = `${OUTPUT}/result.json`;
 const REVIEW_STATE_PATH = `${OUTPUT}/review-state.json`;
 const AGENT_BUDGET_MS = Number(process.env.AGENT_BUDGET_MS);
@@ -829,7 +831,7 @@ function buildRetryScaffold(slugs: readonly string[]): string {
 const ENVELOPE_MISMATCH_EXIT = 42;
 const SUPPORTED_SCHEMA_VERSION = 1;
 const SUPPORTED_KIND = "practice_review";
-const TASK_PATH = "/workspace/task.json";
+const TASK_PATH = `${CWD}/task.json`;
 
 function readTaskEnvelope(): TaskEnvelope {
 	let raw: string;

server/src/test/java/de/tum/cit/aet/hephaestus/agent/handler/PullRequestReviewHandlerTest.java

@@ -24,6 +24,7 @@
 import de.tum.cit.aet.hephaestus.agent.handler.spi.JobSubmission;
 import de.tum.cit.aet.hephaestus.agent.handler.spi.JobSubmissionRequest;
 import de.tum.cit.aet.hephaestus.agent.job.AgentJob;
+import de.tum.cit.aet.hephaestus.agent.runtime.SandboxLayout;
 import de.tum.cit.aet.hephaestus.agent.task.TaskEnvelopeWriter;
 import de.tum.cit.aet.hephaestus.evidence.ArtifactSourceManifest;
 import de.tum.cit.aet.hephaestus.evidence.AutomatedReviewReadinessReport;
@@ -296,6 +297,29 @@ void delegatesToWorkspaceContextBuilder() {
             assertThat(((ContextRequest.PracticeReviewRequest) captor.getValue()).job()).isSameAs(job);
         }
 
+        /**
+         * The composer only runs when this file is staged, and the runner is the only thing that reads it.
+         * Asserted over what preparation actually produces, because a test that calls the staging helper
+         * directly passes whether or not a review ever calls it.
+         */
+        @Test
+        void stagesTheRequestThatTurnsFeedbackCompositionOn() {
+            stubDefaults();
+
+            Map<String, byte[]> files = handler.prepareInputs(jobWithMetadata(sampleJobMetadata())).files();
+
+            assertThat(files)
+                .as("a live review composes feedback, so the request must reach the sandbox")
+                .containsKey(SandboxLayout.FEEDBACK_COMPOSITION_PATH);
+            JsonNode request = objectMapper.readTree(
+                new String(files.get(SandboxLayout.FEEDBACK_COMPOSITION_PATH), StandardCharsets.UTF_8)
+            );
+            assertThat(request.path("enabled").asBoolean()).isTrue();
+            assertThat(request.path("inContextPlacementKinds"))
+                .as("a pull request has a diff, so a note may be placed on one")
+                .anySatisfy(kind -> assertThat(kind.asString()).isEqualTo("DIFF"));
+        }
+
         @Test
         void mergesProviderFiles() {
             byte[] metadataBytes = "{\"pr_number\":42}".getBytes(StandardCharsets.UTF_8);

server/src/test/java/de/tum/cit/aet/hephaestus/agent/practice/live/PracticeRunnerLiveLlmTest.java

@@ -72,7 +72,10 @@ class PracticeRunnerLiveLlmTest {
     ).toAbsolutePath();
 
     /** The runner reads/writes here verbatim; we own the directory for the duration of one test. */
-    private static final Path WORKSPACE = Path.of("/workspace").toAbsolutePath();
+    // Somewhere writable, rather than a root-owned /workspace this harness may not be able to create.
+    private static final Path WORKSPACE = Path.of(
+        System.getenv().getOrDefault("PI_RUNNER_CWD", "/workspace")
+    ).toAbsolutePath();
 
     /** Wall-clock cap for the whole runner — initial + retry budgets are derived from this. */
     private static final long AGENT_BUDGET_MS = 240_000L;
@@ -333,6 +336,8 @@ private void stageWorkspace(LiveLlmCredentials creds) throws IOException {
         // PullRequestContentSource materialises in production.
         Path contextDir = WORKSPACE.resolve(SandboxLayout.CONTEXT_PREFIX);
         Files.createDirectories(contextDir);
+        // Every citation is checked against this; the runner refuses to start without it.
+        copyFixture("manifest.json", WORKSPACE.resolve("inputs").resolve("manifest.json"));
         copyFixture("diff.patch", contextDir.resolve("diff.patch"));
         copyFixture("metadata.json", contextDir.resolve("metadata.json"));
         copyFixture("comments.json", contextDir.resolve("comments.json"));
@@ -411,6 +416,8 @@ private static Process spawnRunner(LiveLlmCredentials creds) throws IOException
         env.put("PATH", path);
         env.put("HOME", WORKSPACE.resolve(".home").toString());
         env.put("LANG", "C.UTF-8");
+        // The only name carried through the clear() above.
+        env.put("PI_RUNNER_CWD", WORKSPACE.toString());
         env.put("LLM_PROXY_URL", creds.baseUrl());
         env.put("LLM_PROXY_TOKEN", creds.apiKey());
         // PI_CODING_AGENT_DIR points Pi at our staged extension + settings, away from ~/.pi.

server/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/SandboxLayoutSyncTest.java

@@ -26,8 +26,12 @@ void runnerLiteralsMatchAbi() throws IOException {
         String body = Files.readString(runner, StandardCharsets.UTF_8);
 
         assertThat(body)
-            .as("runner reads /workspace/task.json")
-            .contains("\"" + SandboxLayout.WORKSPACE_ROOT + "/" + SandboxLayout.TASK_ENVELOPE_FILENAME + "\"");
+            .as("runner defaults its root to SandboxLayout.WORKSPACE_ROOT")
+            .contains("\"" + SandboxLayout.WORKSPACE_ROOT + "\"");
+
+        assertThat(body)
+            .as("runner reads the task envelope at SandboxLayout.TASK_ENVELOPE_FILENAME")
+            .contains("/" + SandboxLayout.TASK_ENVELOPE_FILENAME);
 
         assertThat(body)
             .as("runner pins SUPPORTED_SCHEMA_VERSION to the Java SCHEMA_VERSION constant")
@@ -39,7 +43,7 @@ void runnerLiteralsMatchAbi() throws IOException {
 
         assertThat(body)
             .as("runner writes its output under SandboxLayout.OUTPUT_PATH")
-            .contains("\"" + SandboxLayout.OUTPUT_PATH + "\"");
+            .contains(SandboxLayout.OUTPUT_PATH.substring(SandboxLayout.WORKSPACE_ROOT.length()));
 
         assertThat(body)
             .as("runner reads the practices index under SandboxLayout.PRACTICES_PREFIX")

server/src/test/resources/agent/live-practice/manifest.json

@@ -0,0 +1,47 @@
+{
+  "contractVersion": "1.0.0",
+  "catalogDigest": "978097f57047bb716457e8b3c44115ef0bb55602dbce4f1a15900a40d70a39b8",
+  "artifactKind": "scm.pull_request",
+  "capturedAt": "2026-01-01T00:00:00Z",
+  "sources": [
+    {
+      "kind": "scm.pull-request.diff",
+      "state": {
+        "availability": "AVAILABLE",
+        "content": "NON_EMPTY"
+      },
+      "artifacts": [
+        {
+          "path": "inputs/context/diff.patch"
+        }
+      ]
+    },
+    {
+      "kind": "scm.pull-request.core",
+      "state": {
+        "availability": "AVAILABLE",
+        "content": "NON_EMPTY"
+      },
+      "artifacts": [
+        {
+          "path": "inputs/context/metadata.json"
+        },
+        {
+          "path": "inputs/context/diff_summary.md"
+        }
+      ]
+    },
+    {
+      "kind": "scm.pull-request.comments",
+      "state": {
+        "availability": "AVAILABLE",
+        "content": "EMPTY"
+      },
+      "artifacts": [
+        {
+          "path": "inputs/context/comments.json"
+        }
+      ]
+    }
+  ]
+}