sources: preserve cursor when inbox persistence fails - #410
Open
fallintoplace wants to merge 3 commits into
Open
sources: preserve cursor when inbox persistence fails#410fallintoplace wants to merge 3 commits into
fallintoplace wants to merge 3 commits into
Conversation
alex-clickhouse
added a commit
that referenced
this pull request
Sep 2, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
added a commit
that referenced
this pull request
Sep 3, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
added a commit
that referenced
this pull request
Sep 3, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
added a commit
that referenced
this pull request
Sep 3, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
added a commit
that referenced
this pull request
Sep 3, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alex-clickhouse
added a commit
that referenced
this pull request
Sep 4, 2026
Review findings. Four of these let something be observed that the stated
policy says would not be.
A malformed allow list granted everything it looked like it disabled.
`allow_conversations: {"*": false}` survives the generic coercion as a dict,
and `list()` of a dict yields its keys — so a rule that reads as a switched-off
wildcard became `["*"]`. Mappings are now discarded outright. A bare scalar
still wraps to one pattern, because that is how `${VAR}` arrives and the
repo's own coercion invariant requires it; the first attempt at this fix
dropped it and `test_config_env` caught the regression.
Slack multi-person DMs were observed. They arrive as `channel_type="mpim"` on
a `G` id, so testing for `D` alone filed a group DM away as if it were a
channel. The check now reads the raw declared type rather than the caller's
derived one — that default turns an absent type into "channel", which is
exactly the ambiguity a `G` has to refuse rather than resolve.
Telegram chat titles granted access. A title is set by whoever runs the group,
so `allow_conversations: ["ops-room"]` admitted any group that named itself
that. Titles and usernames are now `self_set_names`, which the access module
already defines as deny-eligible only — the distinction existed and this code
was not using it. Other bots are skipped too, matching Slack's
`_is_another_app_talking`.
Telegram observation now also needs `include_unauthorized_senders`, off by
default. Its only seen-but-unanswered path is a sender the allowlist refused,
so collecting there is a sharper edge than Slack's "in the room but not
talking to me" and should be a decision rather than a side effect.
`ChannelSource` is now `<channel>:observed`. Sharing the bare `telegram` name
with the existing pull source meant sharing a cron job id and a cursor key:
the two runners evicted each other from the scheduler and then read each
other's cursor, one an integer and the other Telethon's JSON state.
Also: valid JSON that is not an object (`[]`) parsed and then raised in
`_to_record`, wedging the cursor behind it; an unusable `schedule` fell back
to nothing, leaving the spool filling with no drain; and the row-cap counter
was process-local, so a daemon restarting more often than it wrote 100 rows
never enforced the cap at all. Spooled text is now capped per row, since
capping row count alone bounds nothing in bytes.
The docs claimed a downstream inbox guardrail that was never wired. The drain
now re-applies the deny rules, which catches a conversation denied after its
messages were already spooled — deny-only, because these rules match id
fields and a name-based allow rule would fail closed and drop everything. The
threat model is restated honestly: nothing here inspects content, and no
allow/deny list separates a report from an instruction.
Not fixed here, and called out in the PR: SourceRunner advances its cursor
when an inbox write fails, which for observations loses them for good. That
is PR #410's subject and belongs there, not duplicated in this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
1 participant
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.
What changed
Why
A bad row used to be logged and skipped. The rest of the batch still committed and the cursor moved forward, so the bad row could be lost permanently.
Tests
uv run pytest tests/test_source_runner_persistence.py tests/test_source_filters.py tests/test_source_resurface.py tests/test_db_atomicity.pyuv run python -m compileall -q nerve/db/sources.py nerve/sources/runner.py tests/test_source_runner_persistence.py