Repository navigation
Conversation
…tic record Collection Profile 0.2.0 lets a connector report what it saw with OBSERVATION messages (Section 5.10), and lets a failed DONE cite them with `error.basis` (Section 5.8). The runtime had no handler, so START did not advertise the capability and a connector could not use it. The runtime now: - advertises OBSERVATION in START and validates the manifest's profile declaration (protocol_version, capabilities, diagnostic_descriptors) before spawn; - accepts or rejects each OBSERVATION without failing the run. A rejection is recorded as a runtime defect. Raw OBSERVATION lines never reach the trace; - applies the strict DONE error shape to a 0.2.0 manifest: failed DONE needs `error` with a boolean `retryable`, `cancelled` is not a connector status, and `basis` may cite only ids the connector emitted. An invalid DONE keeps its bounded connector message visible; - stores one bounded diagnostic record per run (observation snapshot and diagnosis, 8 runs per connector) in `connector_observation_runs`, on SQLite and PostgreSQL, and serves it to the owner session at `/_ref/runs/:run_id/observations`. The Section 5.11 recovery-hint gate is implemented in `observations.ts` and covered by its unit tests. Wiring it to DONE and SKIP_RESULT hints changes retry behavior and lands separately. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
tnunamak
marked this pull request as ready for review
November 5, 2026 19:38
…l id ledger The emitted-id ledger stops at the fact limit. A connector that emitted more observations than the ledger holds could cite an id the ledger had refused, and the runtime then rejected the whole DONE as citing an id it did not emit. That turned the connector's own terminal outcome into a protocol failure. After the ledger overflows, a cited id missing from it can no longer be shown false. The validator now drops that id from the stored basis and keeps the DONE. Before overflow, an unknown id is still rejected. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
The runtime wrote the owner diagnostic record after it emitted the terminal run event. A reader who saw the terminal event could find no record, and a crash between the two writes lost the observed facts for good. Both terminal paths now store the record first: the message-failure path and the child-close path. The write is memoized, so the later settle points stay no-ops. A failed store write is still reported as progress and never changes the run outcome. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
A failed DONE that cited an authentication fact still produced an auth_rejected cause in the owner record when the owner cancelled the run before it closed. The run ended by the owner's choice, so the record named a cause the connector did not end the run with. The record now ignores the DONE basis when the owner requested the cancellation. The facts stay in the record; only the cause is withheld. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
This was referenced Nov 5, 2026
An independent audit of the OBSERVATION intake and its diagnostic store found several places where bad input cost a run more than it should, and where stored records were trusted too much. Intake (runtime/observations.ts): - Invalid, oversize and runtime-typed messages no longer use the rate window or the fact limit. Validation runs first, and only a valid fact that a limit refuses takes a rate slot or marks the record truncated. Before, 256 messages with bad `fact` values and distinct ids could fill the id ledger and the rate window, so the connector's next valid facts were discarded as `count_exceeded`. - The ids of discarded messages go in their own bounded set. An accepted fact keeps its id in the fact map, which the fact limit already bounds, so `DONE.error.basis` can always cite an accepted fact. - A line that is not JSON no longer marks the record truncated, however long it is. One 3 KB garbage line used to block every later held hint for the run. The `oversize` reason is still recorded. - The header no longer claims that intake checks declared id references. Sections 3.8 and 5.10.2 say a fact naming an undeclared step, expectation or rule is kept and supports nothing. Diagnosis already enforces that, and a test pins it. DONE basis (runtime/done-validators.ts): - A basis entry that is not a valid OBSERVATION id now gets its own message, not the "did not emit" message. The comment on the overflow drop now says why the drop cannot change a diagnosis. Store (server/stores/connector-observation-store.ts): - `encodeRecord` and `decodeRow` check each stored fact and defect against the runtime shapes, not only that the arrays exist. The check runs after the size bound. - `list` skips a row it cannot read. Before, one corrupt row made the whole list fail. A direct `get` of that row still fails with `record_invalid`, and unparseable JSON now fails the same way. Route (server/routes/ref-run-observations.ts): - A malformed percent-encoded run id, or an over-long identity, is a 400 `invalid_request`. It was an unhandled error. Tests cover each change, with SQLite and PostgreSQL store contracts. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…ion at the record write An independent audit of the OBSERVATION intake found four defects in this PR's own code. Unparseable lines (runtime/index.ts): - A line that does not parse was classified by a text match on `"type":"OBSERVATION"`. A JSON-escaped spelling of the type missed the match, so the whole line went to the trace and into the parse-error message. Nothing is known about the content of a line that does not parse, so it is never traced (the trace gets its byte count) and neither its text nor the parser message, which can quote the source, goes into an error. The error carries the line length only. - The line is parsed once. The result goes to a single intake, `acceptWire`, which `acceptLine` also uses. Before, the runtime called `accept(undefined, bytes)` for a line that did not parse, which marked the record truncated, while `acceptLine` did not. Cancellation (runtime/index.ts): - The diagnosis was computed from the cancel flag before the record write, and the terminal outcome read the flag again after it. A cancel that landed during the write gave a cancelled run with a stored `auth_rejected` cause. Starting the write now seals the owner-cancel state. A later owner cancel stops the child but does not change the sealed outcome. Store (server/stores/connector-observation-store.ts): - The PostgreSQL write waited on the connector's advisory lock with no bound, and the runtime waits for the write before it publishes a terminal result. The transaction now sets a transaction-local `lock_timeout` and `statement_timeout` (5 s), so a stalled writer fails one optional put and the server cancels the query. Tests: `collection-profile-020` sends raw wire lines (the escaped-type line and a 3 KB non-JSON line) and aborts during the store write; `connector-observation-store` holds the writer lock from another transaction (PostgreSQL); `runtime-observations` checks `acceptWire` against `acceptLine`. Each new test fails before its fix. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
The Section 5.11 gate (gateRecoveryHint, the suggestion record and the hint_unsupported defect) had no caller in this PR. It is code that nothing exercises, so it belongs with the change that wires it to DONE and SKIP_RESULT hints. That change is the next PR in the stack, which now carries the gate, its tests and its caller together. Removed here: gateRecoveryHint and HintGateOutcome, the action and suggestion types, the hint_unsupported defect subtype, the connector_suggestions snapshot field and its persistence validation in the observation store, and the tests for each. The store keeps writing only facts, defects and the truncation flag. The action vocabulary stays owned by RECOVERY_ACTIONS in connector-gap-bounding.ts; the second copy in observations.ts is gone with the gate. No behavior changes for any run: nothing called the gate. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI
… the unparseable-line match Gate 3 review of this PR found that GET /_ref/runs/:runId/observations returned any run's record to any owner session that knew the connector id, the instance id and the run id. Nothing tied the instance to the caller. The sibling abandon-enrollment route already returns 404 when the instance belongs to another owner subject. The route now looks the instance up and answers the same typed 404 when the instance is unknown, belongs to another owner subject, or belongs to another connector. This happens before the record is read. The route context gains getOwnerSubjectId and createRequestConnectorInstanceStore, and server/index.ts passes both. Three new tests failed before the change: a second owner subject read the first owner's record. OBSERVATION_TYPE_TEXT_RE matched "type":"OBSERVATION" anywhere in a line that did not parse, so a log line that quoted the pair counted as a discarded OBSERVATION. It now has to start as a JSON object. A prose line is a protocol violation, as any unparseable line is; a truncated OBSERVATION object is still a discarded OBSERVATION. Both cases have a test, and the prose one failed before. findingOf() keeps its behavior: Collection Profile 0.2.0 makes step and rule optional for a provider_message, so a bare auth_failure supports auth_rejected. A comment cites the sections. Signed-off-by: Tim Nunamaker <tnunamak@gmail.com> Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…something to record persistObservationDiagnosis ran on every terminal path, so every run of every connector, legacy ones included, wrote a connector_observation_runs row. On PostgreSQL the terminal result waits for that write, up to the writer-lock bound. The rows also share the per-connector cap of eight, so empty rows pushed out the one row that explained a failure. The write now happens only when the run's snapshot holds an accepted fact or a defect. The owner-cancel seal still happens at the first settle point. A run with nothing to record has no row; the read route already answers 404 for a missing record. The new test ran a run with a fact, then more silent runs than the cap holds, legacy and declared-but-silent. It failed before: the silent runs had rows and the informative one was evicted. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Base: #625. Merge that first, or retarget this PR to
mainafter it lands.A Collection Profile 0.2.0 connector can send
OBSERVATIONmessages (Section 5.10) and cite them from a failedDONEwitherror.basis(Section 5.8). The runtime now accepts them. It checks each one against the manifest's descriptors, turns a rejected one into a run defect without failing the run, and keeps a per-run diagnostic record that the owner reads atGET /_ref/runs/:run_id/observations.A run writes that record only when it has an accepted OBSERVATION or a defect. A legacy run with neither writes nothing, so it cannot delay a PostgreSQL terminal result or push an informative failure out of the 8-run per-connector cap.
The record is optional reporting: a failed write is a progress event and never changes the run outcome. A remote collector's records are not persisted to the owner server in 0.2.0.
Proved by
collection-profile-020.test.ts, which runs real connector children: ten silent runs, legacy and declared-but-silent, write no row and leave an earlier informative row in place (the test failed before the guard).runtime-observations,connector-observation-storeandref-run-observations-routetests cover validation, the cap and the owner-scoped read.Assisted-by: AI