Skip to content

Read run records, and make a Start leave a mark on the task - #11

Merged
TadMSTR merged 3 commits into
mainfrom
feat/run-records
Aug 29, 2026
Merged

TadMSTR merged 3 commits into
mainfrom
feat/run-records

Conversation

@TadMSTR

@TadMSTR TadMSTR commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Phase 3 of agent-workflow-interop-2026-08, plugin half. Tracker: vikunja#559. Pairs with TadMSTR/task-dispatcher#5.

A Start left no trace on the task

It made no queue mutation at all, so a plugin-started task stayed at approved until its agent got as far as claiming it — and a session that died before that left nothing behind anywhere. That is why one completed steward run was invisible for four days (vikunja#534).

A Start now appends a history entry through the control API. The status is deliberately unchanged — the call re-asserts the status the task is already in, which the handler accepts with allow_override plus a note.

Advancing approved → in-progress here is the obvious-looking alternative and it breaks every plugin-started session: the agent's own first action is update_task(in-progress), which task-queue-mcp permits only from approved. Doing it for the agent means its claim is rejected as an invalid transition.

A failure to record is logged and does not fail the Start — the session is already running by then, and reporting the launch as failed would be the bigger lie.

Run records

This plugin now writes <agent>-<task8>.json beside its log, the same shape the dispatcher writes, and reads both. FORGE_RUN_ID/FORGE_TASK_ID go into the session's environment.

It records a real exit code, and the dispatcher cannot. This is a long-lived process, so the child handle outlives the spawn and 'exit' still fires after unref(). A dispatcher tick spawns a detached child and exits, so its runs can only ever be reaped as pid-gone with a null code. That asymmetry is the design, not an inconsistency. A signalled child records the signal rather than a coerced number.

If CloudCLI restarts before a child ends, the handler never fires and the record stays open — not a leak: the dispatcher's reaper closes it on the next tick, honestly. The two mechanisms are complementary and neither invents an outcome.

The list is now a union

Keyed on the shared <agent>-<task8> stem:

  • A log with no record is one of the 29 runs that predate them. Times still come from the file's mtime; the outcome column reads no run record.
  • A record with no readable log is the security-audit launcher, whose output goes to ~/.pm2/logs/security-audit-<build>.log. That prefix stays outside PREVIEW_ALLOWED_PREFIXES deliberately — it covers every PM2 service log on this host, and adding it would make this endpoint a reader of all of them. So the row renders and the detail view says where the log is. Dropping it would have omitted the commonest kind of headless session here; widening the allowlist to read it would have been a real security regression to fix a display problem.

The outcome column keeps three states apart

Rendered Means
no run record Predates run records; nothing is known
running A record with no ended
exit 0 / exit 137 A real observed code — only this plugin's own launches get one
ended, exit code unknown The run ended and the code is unrecoverable
slot released — still running Past the dispatcher's max runtime; slot freed, process untouched

ended, exit code unknown is not a gap to fill later. Collapsing it into success is the counter-reporting-success failure this whole build exists to expose.

Unchanged, deliberately

  • Status comes from the queue and only from the queue. A record saying a run exited 0 does not promote its task's status. A finished run whose task is still approved is the disagreement this panel exists to show.
  • The <agent>-<task8>.log filename. Two other consumers key on it.

Also

  • Duration for a still-open run is unknown, not a growing number — it would otherwise tick upward on every poll for a session that died an hour ago and has not been reaped.
  • Open runs sort above finished ones. A descending compare on ended put them at the bottom, under three months of finished runs.

Verification

  • 139 tests pass (12 new files' worth of assertions added), tsc clean, vocabulary gate in sync.
  • 14 mutants planted, 14 caught. Two initially escaped and both were weak tests, not weak code — the array-rejection test passed with Array.isArray deleted because the task_id check already excluded arrays (now labelled as intent rather than validation, so nobody later "proves" it load-bearing), and the status test used an open record so a record.ended → completed inference survived it.
  • Run against the real corpus, not just fixtures: 29 files → 27 rows, matching the pre-change behaviour exactly, with the 2 known bare-UUID orphans still skipped.
  • Booted dist/server.js and curled the real endpoints with record fixtures planted in the live launch directory and removed afterwards: the record-only row returned log_readable: false, outcome: "ended, exit code unknown", and its detail route returned 200 with the log's path rather than the pre-change 404.

Deploy

./deploy.sh && pm2 restart cloudcli. dist/ is gitignored and built at deploy time — merged is not deployed, and the panel stays at v0.8.0 until that runs.

🤖 Generated with Claude Code

developer-agent added 3 commits August 29, 2026 10:46
A Start made no queue mutation at all, so a plugin-started task stayed at
`approved` until its agent got as far as claiming it — and a session that died
before that left nothing behind anywhere. That is why one completed steward run
was invisible for four days.

A Start now appends a history entry through the control API, and the status is
deliberately UNCHANGED: the call re-asserts the status the task is already in.
Advancing approved → in-progress is the obvious-looking alternative and it
breaks every plugin-started session, because the agent's own first action is
update_task(in-progress), which task-queue-mcp permits only from `approved`.

The panel now reads the run records both launchers write. The list is the union
of logs and records keyed on their shared stem: a log with no record is one of
the 29 that predate them, and a record with no readable log is the
security-audit launcher, whose output goes to ~/.pm2/logs — a prefix that stays
outside the preview allowlist because it covers every PM2 service log on this
host. That row renders and says where its log is, rather than being dropped;
dropping it would omit the commonest kind of headless session here.

The outcome column keeps three states distinct and does not collapse them.
"ended, exit code unknown" is the honest rendering of a dispatcher launch, not a
gap: a cron tick does not outlive the session it starts. This plugin is
long-lived and can observe its own children exit, so its own launches carry a
real code — that asymmetry is the design.

Status still comes from the queue and only from the queue. A record saying a
run exited 0 does not promote its task.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

agent-id: developer
Found by the pre-audit baseline. Every other read in this file resolves through
resolveAllowedPath; this one did not, because its content is not surfaced
anywhere — which is exactly why it was the copy that got forgotten.

The list route's guard exists because a real ~/.secrets/forge.env symlink was
planted in this directory and read. One rule, every reader, whether or not the
bytes currently go anywhere.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

agent-id: developer
…hat can fail

Both Info findings from the phase34 audit, fixed rather than accepted.

INFO 1 — `mode` was type-asserted off the wire and nothing checked it at
runtime. That was harmless for as long as it only reached `=== 'review'`
comparisons that fell through safely; it stopped being harmless when this
release gave it a consumer that writes it into a task's PERSISTED history note.
toStartMode() now lives beside the type with the closed set exported, so the two
cannot drift. An absent mode still defaults to `review`; a present but
unrecognised one is a 400, because defaulting there would silently downgrade an
operator who asked for `auto` into a session that quietly does nothing.

INFO 2 — closeRunRecord's path guard was added during the pre-audit and had no
regression test. server.ts calls listen() at import time so nothing in it can be
unit-tested, and that is the same property that let the guard be omitted in the
first place: the reader's content is not surfaced anywhere, so nothing was red.
A fix with no test is one refactor from being undone.

loadRunRecord() is now the only reader, extracted to run-record.ts where it can
be tested, and it returns the guard-approved path alongside the record so a
caller writing back uses the path realpath approved rather than re-deriving one
that was never checked. Seven tests, including a real escaping symlink and a
non-escaping one — the guard is about the resolved location, not about symlinks
being forbidden.

5 mutants planted across both fixes, 5 caught. One of them only by tsc rather
than by a test: deleting the route's null check makes `mode` possibly-null where
launchSession wants a StartMode. The validator has tests; the wiring is held by
the type system, which is the most server.ts can offer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

agent-id: developer
@TadMSTR
TadMSTR merged commit f248e05 into main Aug 29, 2026
2 checks passed
@TadMSTR
TadMSTR deleted the feat/run-records branch August 29, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant