refactor(logging): remove the --log-file JSONL run log

[davidslater]

Created by GitHub Ace · View Session

What

Removes --log-file and the separate JSONL run-log artifact. Everything the detector reports now goes to stderr/stdout, so the job log is the single place a run is diagnosed.

Why

The JSONL log was largely a re-encoding of output that already existed:

• conclude — 100% redundant. Every c.logger.Info(...) sat next to a c.info(...) printing the same data to stdout.
• detection run — redundant for artifact_degraded (already ::warning::/::error::), artifacts_all_primary_inputs_missing, artifacts_required_inputs_degraded, custom_prompt_read_failed, prompt_analysis_degraded, engine_create_failed, detection_failed, verdict (= the result JSON), and status (= the THREAT_DETECTION_STATUS: line).

Only five things were genuinely log-only, and those moved to stderr rather than being dropped.

Removed

• --log-file flag, THREAT_DETECTION_LOG_FILE, the derived detection-runlog.jsonl default, and the associated path-collision checks (in both run and conclude)
• the pkg/runlog package
• AnalyzeOptions.Logger plumbing through pkg/engine
• --log-file plumbing and runlog artifact uploads in detection-only.yml and replay-detection.yml

Moved to stderr

[threat-detect] run start: version=… engine=copilot model=… retries=1
[threat-detect] artifacts loaded: dir=… prompt_bytes=… patch_files=1 …
[threat-detect] artifact inventory (4 entries):
[threat-detect]   aw-prompts/prompt.txt bytes=51 kind=file consumed=true
[threat-detect] artifact degraded: field=prompt required_input=true
[threat-detect] prompt built: prompt_bytes=… framework_scaffolding_detected=true …
[threat-detect] detection attempt 1 of 2
[threat-detect] engine argv: "--add-dir" "." …

One subtlety worth calling out: the ::warning:: annotation text alone cannot convey required_input, because in warn mode both required and advisory findings render as ::warning::. TD-18c therefore needed its own explicit line.

Untrusted values (model-authored text, artifact paths, config values) are sanitized to a single physical line, and the inventory listing is bounded at 200 entries, so neither a hostile filename nor model output can forge a workflow command or flood the job log.

Contract

Unchanged: the result JSON (prompt_injection, secret_leak, malicious_patch, reasons), the exit codes, and the THREAT_DETECTION_STATUS: line. Hosts that captured detection output (2>&1 | tee detection.log) keep the full trace; the only loss is the machine-parseable JSONL encoding, which had no consumer.

Docs

Spec is the source of truth, so it moved first: TD-18c, TD-20a (now forbids a separate log artifact and enumerates the required stderr reporting), TD-20c, TD-20d; U-09, U-10, U-18a, U-27 in the usage spec; plus README and CLAUDE.md.

Validation

make fmt-check, lint, build, test, security-govulncheck all pass. Verified end-to-end against a stub engine on both the safe path and the degraded-input/fail-closed path, plus conclude.

Two pre-existing failures are unrelated to this change:

• make golint — golangci-lint built with Go 1.25 vs. the repo's 1.26 target
• make gosec — fails identically on a clean tree (37 findings) and is not run in CI; this change net-reduces findings 37 → 36

[Copilot]

Pull request overview

Removes JSONL run logging and consolidates detector diagnostics into standard output/error and captured job logs.

Changes:

• Removes --log-file, runlog plumbing, package, and tests.
• Adds bounded stderr diagnostics for artifacts, prompts, attempts, and engine invocation.
• Updates workflows, specifications, and documentation.

Show a summary per file

File	Description
specs/usage-spec.md	Updates host logging requirements.
specs/threat-detection-spec.md	Defines stderr-only diagnostics.
README.md	Documents the new logging model.
pkg/runlog/runlog.go	Removes the JSONL logger.
pkg/runlog/runlog_test.go	Removes logger tests.
pkg/engine/log_test.go	Tests stderr invocation diagnostics.
pkg/engine/engine.go	Removes logger plumbing and reports argv.
pkg/artifacts/artifacts.go	Updates warning documentation.
cmd/threat-detect/promptcontext_test.go	Validates prompt context via stderr.
cmd/threat-detect/main.go	Replaces runlog events with stderr output.
cmd/threat-detect/main_test.go	Tests stderr artifact inventory.
cmd/threat-detect/logfile_test.go	Removes JSONL integration tests.
cmd/threat-detect/conclude.go	Removes conclusion runlog support.
cmd/threat-detect/conclude_test.go	Removes conclusion runlog tests.
CLAUDE.md	Removes obsolete runlog guidance.
.github/workflows/replay-detection.yml	Removes replay runlog handling.
.github/workflows/detection-only.yml	Stops producing/uploading JSONL logs.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Suppressed comments (1)

README.md:136

• framework_scaffolding_markers is populated only from knownScaffoldingMarkers (<safe-output-tools>, <github-context>, <agent-context>, and <safe-outputs-config>); the outer <system> tag is never returned. Use an emitted marker (or (none)) in the example.

[threat-detect] prompt built: prompt_bytes=9241 framework_scaffolding_detected=true framework_scaffolding_markers=<system>

• Files reviewed: 17/17 changed files
• Comments generated: 9
• Review effort level: Balanced

[davidslater]

Thanks — reviewed all nine. Eight were correct and are fixed in c87c0ca; one I've scoped rather than "fixed", with reasoning below.

Injection vectors (both real, both fixed)

main.go:164 — unsanitized engine ID. Correct, and the reasoning about ordering is exactly right: the run-start line is emitted before engine.New validates, and Canonical only lowercases, so any --engine value reached it verbatim. Sanitized, along with the version. Verified end-to-end:

$ threat-detect --engine $'copilot\n::error::forged' …
[threat-detect] run start: version=… engine=copilot\n::error::forged model=… retries=1

engine.go:566 — unquoted command. Also correct. nodeCommand() reads GH_AW_NODE_BIN, and the composed description was the one value on that line not quoted while argv and model both were. Now %q.

Both have tests that fail without the fix (I checked by reverting each).

Stale spec references (both mine, both fixed)

TD-18b and TD-22-flags both still required run-log events (prompt_analysis_degraded, prompt_built) that this PR deletes. Clean misses — I updated TD-20a/TD-20c/TD-20d and didn't grep for the events by name. Each is now reduced to its job-log requirement.

Engine output streaming — scoped, not fixed

You're right that runCLIEnvWithSink tees engine stdout/stderr to os.Stderr verbatim, so model output containing \n::warning:: reaches the job log unchanged. But this is pre-existing behavior that predates and is untouched by this PR — the JSONL log never mediated it either — and the teeing is deliberate: it's what makes harness lifecycle output and engine errors visible in real time.

So the defect is in my spec wording, not the code. My TD-20a said "untrusted values echoed into these diagnostics", intending the enumerated detector-composed lines, but it reads as covering everything on stderr. I've scoped it explicitly and stated the passthrough is a separate, non-attested stream, and mirrored that in the README rather than letting the README keep overclaiming. Framing/sanitizing forwarded engine output is a real question, but it's a behavior change to the engine layer that deserves its own PR — happy to open an issue if you'd like.

Lost coverage (both real, both restored)

You're right that deleting logfile_test.go took the only TD-18b assertion with it, even the stderr half. Restored as TestRunWarnsWhenPromptAnalysisArtifactsAreMissing.

And the inventory bound was genuinely untested — I'd added maxInventoryEntries and the sanitization without covering either. TestReportArtifactsBoundsAndEscapesInventory now covers the 200-entry bound, the labelled omission count, the true-total-vs-shown distinction, and a filename with embedded control characters (asserting it cannot open a line).

Replay U-27

Correct catch — I loosened U-27's wording to "captured log" but removed the copy step, leaving the requirement unmet. The step now writes the selected source log to original-detection.log and uploads it.

README example

Both wrong, as flagged: artifactKind emits file (never prompt), and <system> is never in knownScaffoldingMarkers — my own smoke run printed markers=(none) for a prompt that did have a <system> block, which I should have noticed. Example now uses kind=file and real markers (<github-context>, <safe-output-tools>), plus a consumed=false row so the inventory's consumed flag is actually illustrated.

fmt-check, lint, build, test, govulncheck all pass. The pre-existing golint (Go 1.25 vs 1.26 toolchain) and gosec (fails identically on main) failures are unchanged.