Skip to content

Commit 1a68a8d

Browse files
committed
feat(worker): persist trigger attribution on the run record (issue #54)
A forge run's record could join to the triggers.json entry that fired it only by the flow name heuristic, which two triggers naming one flow defeat. The receiver has computed matched: { index, type, ... } since issue #49, and it survives on job.data.trigger all the way into recordRun's argument; buildRecord simply never read it. buildRecord now persists triggerIndex and triggerType as explicit nullable literals beside the replica fields whose admissibility argument they reuse: an integer and a fixed enum, nothing attacker-chosen. The third matched key (label/phrase/action) stays out on purpose, because a label that satisfied an any predicate is collaborator-applied payload text. Cron records keep null for both: their attribution is already exact via the repeat:<id>:<millis> jobId join, which also reaches records written before these fields existed, where a new field cannot. Specs: INT-RUN-HISTORY-FILE-CONTRACT AMENDED (schema, Why, Acceptance: index 0 persists as 0, never null; the matched label never persists). INT-CONTAINER-JOB-INPUTS AMENDED (cross-reference only; matched stays event.json-only inside the container). INT-WEBHOOK-PAYLOAD-SUBSET and INT-OUTBOX-CONTRACT UNCHANGED, checked. Suite run in the CI posture (REQUIRE gates, live Valkey): 2073 pass, 0 skipped. Signed-off-by: Rob Boerman <robboerman@live.nl>
1 parent c5cabbf commit 1a68a8d

4 files changed

Lines changed: 117 additions & 4 deletions

File tree

specs/interfaces.md

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,10 @@ Evidence convention as in `constitution.md`.
683683
`triggers` array (cron entries counted — the file index is the rule's identity), `type` is that
684684
entry's `on.type`, and the third key names what satisfied the rule — for `label` the label that hit
685685
(first `any` hit, else `all[0]`), for `comment` the configured `phrase`, for `pull_request` the
686-
`action`. `matched` does **not** enter the prompt.
686+
`action`. `matched` does **not** enter the prompt. Since issue #54 the harness also persists
687+
`matched.index` and `matched.type` — and only those two — host-side on the run record as
688+
`triggerIndex`/`triggerType` (`INT-RUN-HISTORY-FILE-CONTRACT`); the third key stays in `event.json`
689+
alone, inside the container, because it can carry collaborator-applied text.
687690
- **On a review-triggered job `matched.action` and the record's own `action` deliberately DIFFER**, and
688691
this is the first GitHub case where they do. The record's `event`/`action` pair is byte-for-byte what
689692
GitHub sent (`pull_request_review` / `submitted`); `matched.action` is the `triggers.json` word that
@@ -1776,6 +1779,8 @@ validator rather than a second copy of it.
17761779
"chainRefused": <int> | null, // count of chain requests refused on this parent; 0 = none
17771780
"replica": <int> | null, // this job's 1-based index within its replica set; null = an ordinary run
17781781
"replicas": <int> | null, // the set size, so `r2` is legible without finding the sibling row
1782+
"triggerIndex": <int> | null, // raw triggers-array index of the entry that fired (cron entries counted); forge jobs only
1783+
"triggerType": "label" | "comment" | "pull_request" | null, // that entry's on.type; null on cron, chained, and manual jobs
17791784
"session": { "resumed": <bool>, // what pi ACTUALLY did
17801785
"reason": "<fixed enum: resumed|absent|expired|too-large|unparseable|not-a-regular-file|pi-version-changed|locked|promote-failed|disabled>" | null,
17811786
"bytes": <int> | null } | null } // null when the job had no session at all
@@ -1836,7 +1841,22 @@ validator rather than a second copy of it.
18361841
are **integers**: this record's PII-free-by-construction property rests on it holding no attacker-chosen
18371842
string, and a host-assigned index is not one. The **branch name they imply is deliberately absent**, for
18381843
the same reason `session` omits its key and branch. Without these two fields, two records on one target
1839-
read as an accidental double-run rather than as the pair an operator asked for. The `tokens` field is **additive and nullable**
1844+
read as an accidental double-run rather than as the pair an operator asked for. The trigger-attribution
1845+
fields — `triggerIndex`, `triggerType` — are **additive and nullable on the replica fields' precedent**,
1846+
explicit literals read from the job's own `data.trigger.matched` by the same no-spread `buildRecord`.
1847+
They persist the receiver's harness-computed decision record (`INT-CONTAINER-JOB-INPUTS`):
1848+
`triggerIndex` is the raw `triggers.json` array position of the entry that fired (cron entries counted —
1849+
the file index is the rule's identity), `triggerType` that entry's `on.type`. An **integer and a fixed
1850+
enum are the same admissible class as `replica` and `session.reason`**; the third `matched` key
1851+
(`label`/`phrase`/`action`) is **deliberately absent**, because a label that satisfied an `any`
1852+
predicate is collaborator-applied payload text, and persisting it would put an attacker-adjacent string
1853+
in a record whose PII-free property rests on holding none. Without these two fields a forge run joins to
1854+
its trigger only by the flow-name heuristic, which two triggers naming one flow defeat — the exact
1855+
ambiguity `matched` was minted to remove (issue #49; persisting it was deferred there by that issue's
1856+
no-new-record-fields scope, not by this record's posture). **Cron records hold `null` for both on
1857+
purpose**: a cron job's `data.trigger` is `{ id, pattern }` with no `matched`, and its attribution is
1858+
already exact via the `repeat:<id>:<millis>` jobId join (above) — a join that also reaches records
1859+
written before these fields existed, where a new field cannot. The `tokens` field is **additive and nullable**
18401860
in exactly the same way — an explicit no-spread literal of the runner's per-job usage totals
18411861
(`REQ-TOKEN-ACCOUNTING-AND-CAPS`), or `null` when the container died before emitting the runner `exit`
18421862
line. It is PII-free by construction: integer token counts and a numeric cost only, no
@@ -1894,7 +1914,10 @@ validator rather than a second copy of it.
18941914
charset-validated ids and its rows sum to `tokens.total`; given a usage block violating any field rule,
18951915
the record stores `usage: null`, never a partial; given a catch-path or pre-exit-line death,
18961916
`provider`/`model` still carry the host-effective dispatch values; given a fallback-metered
1897-
(`metered: false`) or pre-ledger run, `usage` is `null` and no reader treats that as an error.
1917+
(`metered: false`) or pre-ledger run, `usage` is `null` and no reader treats that as an error. Given a
1918+
forge job whose data carries `trigger.matched`, the record holds `triggerIndex` and `triggerType` —
1919+
index `0` persists as `0`, never as `null` — and neither field ever carries the matched
1920+
`label`/`phrase`/`action`; given a cron, chained, or manual job, both are `null`.
18981921
18991922
## INT-OUTBOX-CONTRACT
19001923
@@ -2223,6 +2246,7 @@ recorded repair is re-running `/dispatch setup` (or editing the pointer by hand)
22232246
22242247
| Date | Change |
22252248
|---|---|
2249+
| 2026-08-11 | Issue #54 (Gap 2: a forge run's record could not be attributed to the `triggers.json` entry that fired it). **INT-RUN-HISTORY-FILE-CONTRACT AMENDED**: two additive, nullable fields on the replica fields' precedent — `triggerIndex` (the raw triggers-array index of the winning entry, cron entries counted) and `triggerType` (that entry's `on.type`) — explicit literals read from the job's own `data.trigger.matched` by the same no-spread `buildRecord`. An integer and a fixed enum, the admissible class the record already holds; the third `matched` key (`label`/`phrase`/`action`) is deliberately NOT persisted, because a label that satisfied an `any` predicate is collaborator-applied payload text and this record's PII-free property rests on holding no attacker-chosen string. Cron records keep `null` for both on purpose: a cron job's attribution is already exact via the `repeat:<id>:<millis>` jobId join, which also reaches records written before these fields existed, where a new field cannot. Persisting `matched` was deferred by issue #49's own no-new-record-fields scope, not by this record's posture; issue #54 is the consumer that makes it earn its place. Acceptance pins index `0` persisting as `0`, never `null`. **INT-CONTAINER-JOB-INPUTS AMENDED**: one cross-reference — `matched` remains event.json-only inside the container and never enters the prompt; its `index`/`type` alone are now also persisted host-side. **INT-WEBHOOK-PAYLOAD-SUBSET UNCHANGED, checked**: `matched` is harness-computed metadata, not a payload field, so the subset is untouched. **INT-OUTBOX-CONTRACT UNCHANGED, checked**: chained children carry no `trigger` and record `null`/`null`, exactly as manual runs do. |
22262250
| 2026-08-09 | Follow-up audit after issue #60. **INT-SDK-SESSION-OPTIONS AMENDED**, a correction rather than an addition: the option block still showed the TWO-path `additionalSkillPaths` literal, while the prose beside it had already been updated to three protected roots. A contract block that disagrees with its own note is worse than either being wrong alone, since a reader checking the code against the spec would have found the spec confirming the old shape. Now shows repo, injected, overlay. No behaviour changed; the literal had been stale since the injected tier landed hours earlier. |
22272251
| 2026-08-09 | Issue #60 (Gap 3: `run.instructions`). **INT-TRIGGERS-FILE-CONTRACT AMENDED**: a new optional field on the three webhook types, refused on cron with a message naming `run.task`, capped at 2000 characters and refused rather than truncated. Surrounding whitespace is deliberately NOT refused here, unlike `run.image`, and the divergence is recorded: that rule exists because whitespace changes what an image REFERENCE means, and it does not change what prose means. **INT-CONTAINER-JOB-INPUTS AMENDED**: `prompt.md` may now carry an operator standing-instruction block in the envelope above the data region; it reaches no other file. **INT-WEBHOOK-PAYLOAD-SUBSET UNCHANGED, checked**: the field is operator config and is not a webhook body field, so the subset is untouched and the value never appears in `event.json`. |
22282252
| 2026-08-09 | Issue #60 (Gap 2: `run.skillsDir`, a per-trigger operator skills directory). **INT-TRIGGERS-FILE-CONTRACT AMENDED**: a new optional field on all four run kinds, with the validation SPLIT written out because both halves are load-bearing. Existence is not checked in the shared validator because BOTH services parse this file and the receiver may run on another host (the `run.folder` precedent); absoluteness is not checked there either, and that one is subtler, because `path.isAbsolute` is OS-dependent, so a shared check would let a Windows worker and a Linux receiver disagree about the same reviewed file. The worker enforces both where the answer is knowable: at boot for cron, and pre-spend per job for every kind. Also records that the value never reaches `/job/event.json` (it rides at JOB level, never inside `trigger`, which is what the subset is built from), and that injected skills are trigger-reachable and never AI-reachable. **INT-CONTAINER-JOB-INPUTS AMENDED**: `/job/trigger-skills/<name>/**` joins the layout as the one `/job` input that does NOT come from git, with the asymmetry argued rather than left to be noticed -- `.pi/` is read by oid because the serviced repo is only maintainer-trusted and an attacker can shape that tree, while `run.skillsDir` is operator-authored deploy-time config named in a reviewed file, so what remains is the ordinary filesystem hazard and the copier answers it the same way (lstat never stat, regular files only, destinations rebuilt from validated segments, bounded). It arrives on the EXISTING `/job:ro` bind. **INT-RUN-HISTORY-FILE-CONTRACT AMENDED**: six `skills-dir-*` reasons. **INT-SDK-SESSION-OPTIONS AMENDED**: the protected-root list goes to three, `/job/pi/skills` then `/job/trigger-skills` then `/opt/pi-global/skills`, consulted in that order. **INT-CONTAINER-RUNTIME-CONTRACT UNCHANGED, checked** -- and this is the entry the change was designed around: no mount is added, no flag, no env var, so a job with an injected skills dir has a docker argv byte-identical to one without, pinned by a test. |

worker/src/run-history.mjs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,19 @@ export function buildRecord({ job, result, error, startedAt, endedAt }) {
295295
// deliberately not stored, for the reason `session` states one group below.
296296
replica: data.replica ?? null,
297297
replicas: data.replicas ?? null,
298+
// Trigger attribution (INT-RUN-HISTORY-FILE-CONTRACT, issue #54): additive and nullable, explicit
299+
// literals beside the replica fields whose admissibility argument they reuse, no spread. Both read
300+
// the receiver's harness-computed `matched` from this job's own `job.data.trigger`: `index` is the
301+
// raw triggers-array position of the entry that fired (cron entries counted) and `type` that entry's
302+
// `on.type` -- an INTEGER and a FIXED ENUM ("label" | "comment" | "pull_request"), nothing
303+
// attacker-chosen. The third `matched` key (`label`/`phrase`/`action`) is DELIBERATELY absent:
304+
// a label that satisfied an `any` predicate is collaborator-applied payload text, and `type`
305+
// already names the route. Cron jobs carry `trigger: { id, pattern }` with no `matched`, so both
306+
// stay null there on purpose -- a cron run's attribution is already exact via its
307+
// `repeat:<id>:<millis>` jobId (see makeFindPreviousRun), and that join also works retroactively
308+
// over the whole retention window, which a new record field cannot.
309+
triggerIndex: data.trigger?.matched?.index ?? null,
310+
triggerType: data.trigger?.matched?.type ?? null,
298311
// Session telemetry (INT-RUN-HISTORY-FILE-CONTRACT): additive, nullable, an explicit literal, no
299312
// spread. `{ resumed, reason, bytes }` -- a boolean, a fixed enum and an integer. THE KEY AND THE
300313
// BRANCH NAME ARE DELIBERATELY ABSENT: this record's PII-free-by-construction property rests on it

worker/test/run-history.test.mjs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,8 @@ test("buildRecord for a github job keeps id-only fields and admits no PII", () =
407407
assert.equal(record.exitCode, null);
408408
assert.equal(record.budgetReserved, null);
409409
assert.equal(record.reason, null);
410+
assert.equal(record.triggerIndex, null, "a job whose data carries no trigger.matched records null attribution");
411+
assert.equal(record.triggerType, null);
410412

411413
const json = JSON.stringify(record);
412414
assert.ok(!json.includes("SECRET_T"), "title must not leak");
@@ -1002,3 +1004,64 @@ test("the replica fields keep the record PII-free by construction -- integers on
10021004
assert.equal(typeof rec.replica, "number");
10031005
assert.equal(typeof rec.replicas, "number");
10041006
});
1007+
1008+
test("buildRecord persists triggerIndex and triggerType from trigger.matched, and index 0 is 0, never null", () => {
1009+
// Additive and nullable on the replica fields' precedent (INT-RUN-HISTORY-FILE-CONTRACT, issue #54).
1010+
// Index 0 is a LEGAL index -- the first triggers.json entry -- so the `?? null` default must not
1011+
// swallow it; this is the assertion a `|| null` typo would turn red.
1012+
const rec = buildRecord({
1013+
job: {
1014+
id: "gh-guid",
1015+
name: "github",
1016+
data: {
1017+
kind: "github",
1018+
repo: "o/r",
1019+
target: { type: "issue", number: 7 },
1020+
flow: "fix",
1021+
trigger: { kind: "issues", matched: { index: 0, type: "label", label: "SECRET_LABEL" } },
1022+
},
1023+
},
1024+
result: { outcome: "completed" },
1025+
});
1026+
assert.equal(rec.triggerIndex, 0, "index 0 persists as 0 -- the ?? default must not eat it");
1027+
assert.equal(rec.triggerType, "label");
1028+
assert.equal("matched" in rec, false, "the matched OBJECT is never stored -- only its two admissible fields");
1029+
1030+
const json = JSON.stringify(rec);
1031+
assert.equal(json.includes("SECRET_LABEL"), false, "the third matched key is collaborator-applied text and never persists");
1032+
});
1033+
1034+
test("triggerType persists each of the closed route set, and nothing else rides along", () => {
1035+
// The set is minted by the receiver's filters (receiver/src/filter.mjs: label, comment, pull_request;
1036+
// the review route reuses pull_request). A record consumer may switch on these three values exactly.
1037+
for (const type of ["label", "comment", "pull_request"]) {
1038+
const rec = buildRecord({
1039+
job: {
1040+
id: `gh-${type}`,
1041+
name: "github",
1042+
data: { kind: "github", repo: "o/r", target: { type: "issue", number: 1 }, flow: "fix", trigger: { matched: { index: 3, type, phrase: "SECRET_PHRASE" } } },
1043+
},
1044+
result: { outcome: "completed" },
1045+
});
1046+
assert.equal(rec.triggerType, type);
1047+
assert.equal(rec.triggerIndex, 3);
1048+
assert.equal(JSON.stringify(rec).includes("SECRET_PHRASE"), false);
1049+
}
1050+
});
1051+
1052+
test("a cron-shaped trigger ({id, pattern}, no matched) records null attribution on purpose", () => {
1053+
// Cron attribution is already exact via the repeat:<id>:<millis> jobId join (makeFindPreviousRun),
1054+
// which also reaches records written before these fields existed. Persisting trigger.id here would
1055+
// duplicate a fact the record's own jobId carries.
1056+
const rec = buildRecord({
1057+
job: {
1058+
id: "repeat:nightly:1754870400000",
1059+
name: "local",
1060+
data: { kind: "local", folder: "/x/proj", flow: "tidy", trigger: { id: "nightly", pattern: "0 3 * * *" } },
1061+
},
1062+
result: { outcome: "completed" },
1063+
});
1064+
assert.equal(rec.triggerIndex, null);
1065+
assert.equal(rec.triggerType, null);
1066+
assert.equal(JSON.stringify(rec).includes("nightly"), true, "the id still reaches the record -- inside jobId, its canonical home");
1067+
});

worker/test/wiring.test.mjs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,11 +344,19 @@ function makeRealRecordRun({ writeThrows = false } = {}) {
344344
}
345345

346346
// The full BullMQ job wrapper carrying user-authored PII (title/body) the record must never serialise.
347+
// trigger.matched rides along so (a) can prove the attribution SPLIT end-to-end: index/type persist,
348+
// the collaborator-applied label never does (INT-RUN-HISTORY-FILE-CONTRACT, issue #54).
347349
const secretJob = (id = "j1") => ({
348350
id,
349351
attemptsMade: 0,
350352
name: "github",
351-
data: { kind: "github", repo: "o/r", flow: "fix", target: { type: "issue", number: 1, title: "SECRET_T", body: "SECRET_B" } },
353+
data: {
354+
kind: "github",
355+
repo: "o/r",
356+
flow: "fix",
357+
target: { type: "issue", number: 1, title: "SECRET_T", body: "SECRET_B" },
358+
trigger: { kind: "issues", matched: { index: 0, type: "label", label: "SECRET_LABEL" } },
359+
},
352360
});
353361

354362
// A processor wired to the real recordRun. `runContainer`/`redis` are overridable so the infra-exit
@@ -396,9 +404,14 @@ test("(a) completed run: real writer serialises a PII-free record to <jobId>.jso
396404
assert.equal(rec.provider, "anthropic", "provider is the overlay-resolved host fact, never a container string");
397405
assert.equal(rec.model, "m");
398406
assert.equal(rec.usage, null);
407+
// Trigger attribution rides the serialized bytes end-to-end, split exactly as the contract says:
408+
// the integer and the enum persist, the collaborator-applied label does not.
409+
assert.equal(rec.triggerIndex, 0, "matched.index persists, and index 0 is 0, never null");
410+
assert.equal(rec.triggerType, "label");
399411
// buildRecord reads only stable non-PII fields, so the serialized bytes carry neither title nor body.
400412
assert.equal(writes[0].data.includes("SECRET_T"), false, "issue title must not leak into the record bytes");
401413
assert.equal(writes[0].data.includes("SECRET_B"), false, "issue body must not leak into the record bytes");
414+
assert.equal(writes[0].data.includes("SECRET_LABEL"), false, "the matched label must not leak into the record bytes");
402415
});
403416

404417
test("(b) infra exit 1: a failed record is written on the catch path BEFORE InfraRetry rethrows", { skip }, async () => {

0 commit comments

Comments
 (0)