yuris/life-service
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages 1 Wiki Activity Actions

feat(policy): explicit blockers as structured linked context and the derived active set #79

Merged
yuris merged 2 commits from feat/issue-63-blockers into main 2026-09-23 05:15:46 +00:00
Conversation 2 Commits 2 Files changed 10 +1591 -23
yuris commented 2026-09-23 04:49:33 +00:00
Owner
Copy link

Motivation

docs/technical/domain-model.md#task-axes states the contract: "Blockers are explicit structured user facts in linked context, never a fabricated standard STATUS named BLOCKED."

#22 shipped only the matching half of it: FilterContext.active_blockers excludes a candidate whose canonical composite item id is in the set, and that exclusion is unit-tested — but nothing in the tree ever derived the set, so in practice it was always empty and the boundary never fired end to end. This PR records the fact, derives the set from the journal, and lets a user resolve a blocker explicitly.

Changes

Recording the fact — src/api/context.rs

POST /v1/context (and therefore life context add --input -) accepts one new optional member, blocked_by, naming the item that blocks the context's item_id by canonical composite id (itm_…). The assertion "item_id is blocked by blocked_by" is recorded as one context fact with a structured link:

{"text": "…", "blocker": {"blocked_item_id": "itm_…", "blocking_item_id": "itm_…"}}
  • blocked_item_id is the canonical id the live read established for item_id (resolve_live_item), not the caller's string, so the fact names exactly the identity the policy filter matches.
  • The response echoes data.blocker, so the caller sees what was recorded; life context add prints it as Blocked by: … is blocked by ….
  • Refused with 422 invalid_input before any write: blocked_by without item_id (a link needs both ends), a blocked_by that is not a well-formed item id, and a self-link. "Well-formed" is the item routes' own rule, now stated once in src/api/types.rs::is_well_formed_id and used by src/api/items.rs, the blocker link and the derivation: itm_ plus at least eight RFC 4648 base32 characters (the alphabet item_id_for emits). Prefix-only values (itm_), punctuation (itm_!), hyphens (itm_not-an-id), a lowercase alphabet, a too-short suffix (itm_ABC) and any other prefix are refused, so a typo cannot be recorded as a fact. A blocked item that is not live stays the route's existing not_found.
  • Idempotency is the route's existing one: the stated link joins the semantic body, so the same key with the same body replays the recorded fact and the same key with a different body is idempotency_conflict. A context that states no link keeps a byte-identical fingerprint, so previously recorded operations are unaffected.

Deriving the set — src/history/blockers.rs (new)

active_blockers(db) is one read over the append-only journal: life_events rows of kind context_captured that carry the structured link, with both ends passing is_well_formed_id, minus every fact the journal's own supersession ledger records as replaced. A row whose ends are malformed is ignored, not trusted — the write path refuses to create one, so a row that has one anyway is not a blocker fact, and failing the whole read over it would deny every next action instead of one task. Nothing else can produce a blocker — no prose, no model output, no external link.

Wiring — src/api/next.rs, src/api/plans.rs

recorded_blockers() fills FilterContext.active_blockers for POST /v1/next and POST /v1/plans. No second exclusion reason was added: the reason that fires is #22's ExclusionReason::Blocked (reason: "blocked", message "task is blocked by linked context"), and it is observable in the response's excluded list. A journal that cannot be read yields the empty set plus the documented history_incomplete warning — "nothing is blocked" is a claim, and a read that did not happen does not establish it.

Resolution — #18's supersession, not age and not silence

A blocker clears when a later fact names the blocker fact as superseded (supersedes_context_id, the mechanism #18 shipped). The journal stays append-only: the original row is untouched, the resolution is a new fact, and active_blockers folds over the chain.

The shape rule, promoted — src/api/types.rs, src/api/items.rs

is_well_formed_id moves beside item_id_for (with the base32 alphabet now a single BASE32_ALPHABET const that both it and base32_push use), so the item routes, the blocker link and the derivation share one rule instead of the item routes having one and this feature having a second, weaker one.

Docs — skills/life/references/commands.md

The context add row now names blocked_by; nothing else in the skill package changed (no row moved between implemented and not-implemented, so tests/skill_package_tests.rs counts are unchanged).

Verification

$ nix develop --command cargo fmt --check
# clean: no output

$ nix develop --command cargo test
# 41 suite-result lines, every one `ok`, no failures:
# test result: ok. 755 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
# (summed across the 41 `test result:` lines: 755 passed, 0 failed)

$ nix develop --command cargo clippy --all-targets -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 25.97s
# 0 errors, 0 warnings

The baseline on main (37c8101) is 743 tests; this PR adds 12, all in tests/issue63_blockers.rs, and changes no existing test.

New integration suite tests/issue63_blockers.rs (12 tests) — every assertion goes through the real path (mock CalDAV over TCP, the daemon's own router, the daemon's own SQLite journal, the real CLI binary against a served daemon):

Test Criterion it defends
a_blocker_is_recorded_as_a_structured_fact_and_replays_under_its_key the fact is structured and journaled; same key replays, different body conflicts
a_blocker_link_needs_both_ends_and_well_formed_item_ids one-ended, self, free-text and every malformed-id form (itm_!, itm_not-an-id, lowercase, itm_, itm_ABC, another prefix) are refused with no journal and no outbox row, and a generated id records and excludes (positive control)
a_malformed_journal_row_never_produces_a_blocker a hand-written row with a malformed end is ignored by the derivation, while the same row with a well-formed end does exclude
a_blocked_task_is_excluded_from_next_with_an_observable_reason end to end: the recorded task is out with reason blocked, the blocking task stays a candidate
only_the_recorded_canonical_item_id_is_blocked the derived set holds exactly the recorded item, by canonical composite id
recorded_prose_never_produces_a_blocker prose recorded through the same route is not a blocker
a_model_judgment_never_produces_a_blocker accepted model output (basis: semantic) creates no blocker
an_unresolved_external_related_to_link_never_produces_a_blocker an unresolved RELATED-TO link is not a blocker
a_resolved_blocker_stops_excluding_the_task_and_is_journaled resolution via supersession: exclusion stops, both facts stand
the_cli_reports_the_blocked_exclusion_in_its_envelope life context add records the link and life next --json reports the exclusion
the_human_view_names_the_blocked_exclusion the human life next view names the excluded task and the reason
the_calendar_is_only_ever_read neither recording a blocker nor asking for a next action writes to the calendar

grep -ri "blocked" src/ → 49 matches in 8 files, and every one is either the new blocker vocabulary (src/history/blockers.rs, blocked_by/blocked_item_id in src/api/context.rs, the ExclusionReason::Blocked the domain already had, the two CLI/next comment lines) or the domain's own refusal of such a status: src/domain/lifecycle.rs rejects STATUS:BLOCKED as an unknown status (and asserts that refusal), and the Jev readiness question's lowest legend is "Blocked or unclear". No BLOCKED status exists anywhere, in iCalendar, in policy or in a string comparison.

No existing test was changed, weakened or deleted.

Disclosure — choices this PR made where the spec was silent

  1. Request shape. cli-api.md documented Context add as text, optional item_id, reason, supersedes_context_id and said nothing about blockers. The minimal shape consistent with that body is one new optional member, blocked_by, on the existing item_id (the blocked item): the link reuses the item the route already resolves live rather than introducing a second item reference or a kind discriminator. docs/technical/cli-api.md gained the field in that row plus a one-paragraph "Blocker context:" note beside the existing payload paragraphs, in the page's own style. docs/ is excluded from this repository's git (.git/info/exclude), so that spec edit is local-only and cannot appear in this PR — it is called out here so the choice is reviewable.
  2. The two ends are validated asymmetrically. The blocked item must resolve live (it is the id the exclusion matches and the item the memory is tagged to); the blocking item is validated for canonical shape only. Requiring it live would make recording a blocker fail whenever the blocking item has since been deleted or completed, which is exactly when a user is likely to be recording the relationship.
  3. history_incomplete is reused for an unread blocker set. cli-api.md#error-and-warning-identifiers is the closed list of non-error warning identifiers and this is the one it documents for "the journal could not be read"; no new identifier was invented.
  4. The planning route (POST /v1/plans) reads the same set. The issue names #24 (life next) for the wiring; leaving plans on a different input would have made the same policy filter reach two different candidate sets, so the three-line wiring is included and disclosed here as slightly beyond the literal issue text.
  5. Human life next prints the exclusion list only when no candidate is eligible — the renderer's existing policy. The exclusion reason is always in the envelope (--json), and the_human_view_names_the_blocked_exclusion covers the empty-candidate rendering.
  6. Field validation answers 422 invalid_input (validation_failed), matching the sibling body-field validation in the same function (require_text) rather than the route's transport-level 400s.

Review response

The first revision of this PR carried a permissive is_canonical_item_id (prefix plus "no whitespace"), which accepted itm_! and itm_not-an-id; the reviewer reproduced both and the PR-body sentence claiming that a "non-canonical blocked_by" was refused was therefore false. This revision deletes that helper, promotes the item routes' existing well-formed-id rule so both callers share it, refuses every malformed form at the route (422, no journal row, no outbox row), makes the derivation apply the same rule to a row it reads, and covers each form plus a positive control in a_blocker_link_needs_both_ends_and_well_formed_item_ids and a_malformed_journal_row_never_produces_a_blocker. The two overstated sentences are rewritten above.

Defects found outside this PR's scope (reported, not fixed)

  1. history_unavailable is emitted but not documented. src/api/items.rs:179 and src/api/context.rs (four sites) push a history_unavailable warning, but docs/technical/cli-api.md#error-and-warning-identifiers is the closed list of non-error warning identifiers and does not contain it. This PR deliberately reuses the documented history_incomplete rather than adding to that drift, but the existing mismatch is a real one and is left alone here.
  2. One failure code, two HTTP statuses. ApiFailure::invalid_input answers 400 and ApiFailure::validation_failed answers 422, and both serialize the error code invalid_input (src/api/mod.rs). The consequence is visible inside one function: POST /v1/context answers 400 for an empty supersedes_context_id and 422 for the sibling field validations (require_text, and the new blocked_by checks). Not touched here — it is a route-wide decision, not this issue's.
  3. docs/ is not version-controlled in this repository. .git/info/exclude excludes it, so the specification this work was designed against (docs/technical/*.md, including the cli-api.md paragraph this PR's request shape is documented in) exists only on the working machine and never reaches a branch or a reviewer. The spec edits for this change are therefore local-only; this is called out because it makes "the spec is the source of truth" unenforceable by review as the repository is currently configured.

Related Issues

Closes #63
Refs #22 (the matching half this completes), #18 (the supersession mechanism resolution uses), #24 (the command path the exclusion is observable on)

## Motivation `docs/technical/domain-model.md#task-axes` states the contract: *"Blockers are explicit structured user facts in linked context, never a fabricated standard STATUS named BLOCKED."* #22 shipped only the **matching** half of it: `FilterContext.active_blockers` excludes a candidate whose canonical composite item id is in the set, and that exclusion is unit-tested — but nothing in the tree ever *derived* the set, so in practice it was always empty and the boundary never fired end to end. This PR records the fact, derives the set from the journal, and lets a user resolve a blocker explicitly. ## Changes **Recording the fact — `src/api/context.rs`** `POST /v1/context` (and therefore `life context add --input -`) accepts one new optional member, `blocked_by`, naming the item that blocks the context's `item_id` by canonical composite id (`itm_…`). The assertion "`item_id` is blocked by `blocked_by`" is recorded as **one context fact** with a structured link: ```json {"text": "…", "blocker": {"blocked_item_id": "itm_…", "blocking_item_id": "itm_…"}} ``` - `blocked_item_id` is the canonical id the **live read** established for `item_id` (`resolve_live_item`), not the caller's string, so the fact names exactly the identity the policy filter matches. - The response echoes `data.blocker`, so the caller sees what was recorded; `life context add` prints it as `Blocked by: … is blocked by …`. - Refused with 422 `invalid_input` before any write: `blocked_by` without `item_id` (a link needs both ends), a `blocked_by` that is not a **well-formed item id**, and a self-link. "Well-formed" is the item routes' own rule, now stated once in `src/api/types.rs::is_well_formed_id` and used by `src/api/items.rs`, the blocker link and the derivation: `itm_` plus at least eight RFC 4648 base32 characters (the alphabet `item_id_for` emits). Prefix-only values (`itm_`), punctuation (`itm_!`), hyphens (`itm_not-an-id`), a lowercase alphabet, a too-short suffix (`itm_ABC`) and any other prefix are refused, so a typo cannot be recorded as a fact. A blocked item that is not live stays the route's existing `not_found`. - Idempotency is the route's existing one: the stated link joins the semantic body, so the same key with the same body replays the recorded fact and the same key with a different body is `idempotency_conflict`. A context that states no link keeps a byte-identical fingerprint, so previously recorded operations are unaffected. **Deriving the set — `src/history/blockers.rs` (new)** `active_blockers(db)` is one read over the append-only journal: `life_events` rows of kind `context_captured` that carry the structured link, with **both ends passing `is_well_formed_id`**, minus every fact the journal's own supersession ledger records as replaced. A row whose ends are malformed is **ignored, not trusted** — the write path refuses to create one, so a row that has one anyway is not a blocker fact, and failing the whole read over it would deny every next action instead of one task. Nothing else can produce a blocker — no prose, no model output, no external link. **Wiring — `src/api/next.rs`, `src/api/plans.rs`** `recorded_blockers()` fills `FilterContext.active_blockers` for `POST /v1/next` and `POST /v1/plans`. No second exclusion reason was added: the reason that fires is #22's `ExclusionReason::Blocked` (`reason: "blocked"`, message "task is blocked by linked context"), and it is observable in the response's `excluded` list. A journal that cannot be read yields the empty set **plus** the documented `history_incomplete` warning — "nothing is blocked" is a claim, and a read that did not happen does not establish it. **Resolution — #18's supersession, not age and not silence** A blocker clears when a later fact names the blocker fact as superseded (`supersedes_context_id`, the mechanism #18 shipped). The journal stays append-only: the original row is untouched, the resolution is a new fact, and `active_blockers` folds over the chain. **The shape rule, promoted — `src/api/types.rs`, `src/api/items.rs`** `is_well_formed_id` moves beside `item_id_for` (with the base32 alphabet now a single `BASE32_ALPHABET` const that both it and `base32_push` use), so the item routes, the blocker link and the derivation share one rule instead of the item routes having one and this feature having a second, weaker one. **Docs — `skills/life/references/commands.md`** The `context add` row now names `blocked_by`; nothing else in the skill package changed (no row moved between implemented and not-implemented, so `tests/skill_package_tests.rs` counts are unchanged). ## Verification ``` $ nix develop --command cargo fmt --check # clean: no output $ nix develop --command cargo test # 41 suite-result lines, every one `ok`, no failures: # test result: ok. 755 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out # (summed across the 41 `test result:` lines: 755 passed, 0 failed) $ nix develop --command cargo clippy --all-targets -- -D warnings Finished `dev` profile [unoptimized + debuginfo] target(s) in 25.97s # 0 errors, 0 warnings ``` The baseline on `main` (`37c8101`) is 743 tests; this PR adds 12, all in `tests/issue63_blockers.rs`, and changes no existing test. New integration suite `tests/issue63_blockers.rs` (12 tests) — every assertion goes through the real path (mock CalDAV over TCP, the daemon's own router, the daemon's own SQLite journal, the real CLI binary against a served daemon): | Test | Criterion it defends | |---|---| | `a_blocker_is_recorded_as_a_structured_fact_and_replays_under_its_key` | the fact is structured and journaled; same key replays, different body conflicts | | `a_blocker_link_needs_both_ends_and_well_formed_item_ids` | one-ended, self, free-text and every malformed-id form (`itm_!`, `itm_not-an-id`, lowercase, `itm_`, `itm_ABC`, another prefix) are refused with no journal and no outbox row, and a generated id records and excludes (positive control) | | `a_malformed_journal_row_never_produces_a_blocker` | a hand-written row with a malformed end is ignored by the derivation, while the same row with a well-formed end does exclude | | `a_blocked_task_is_excluded_from_next_with_an_observable_reason` | end to end: the recorded task is out with reason `blocked`, the blocking task stays a candidate | | `only_the_recorded_canonical_item_id_is_blocked` | the derived set holds exactly the recorded item, by canonical composite id | | `recorded_prose_never_produces_a_blocker` | prose recorded through the same route is not a blocker | | `a_model_judgment_never_produces_a_blocker` | accepted model output (`basis: semantic`) creates no blocker | | `an_unresolved_external_related_to_link_never_produces_a_blocker` | an unresolved `RELATED-TO` link is not a blocker | | `a_resolved_blocker_stops_excluding_the_task_and_is_journaled` | resolution via supersession: exclusion stops, both facts stand | | `the_cli_reports_the_blocked_exclusion_in_its_envelope` | `life context add` records the link and `life next --json` reports the exclusion | | `the_human_view_names_the_blocked_exclusion` | the human `life next` view names the excluded task and the reason | | `the_calendar_is_only_ever_read` | neither recording a blocker nor asking for a next action writes to the calendar | `grep -ri "blocked" src/` → **49 matches in 8 files**, and every one is either the new blocker vocabulary (`src/history/blockers.rs`, `blocked_by`/`blocked_item_id` in `src/api/context.rs`, the `ExclusionReason::Blocked` the domain already had, the two CLI/next comment lines) or the domain's own refusal of such a status: `src/domain/lifecycle.rs` rejects `STATUS:BLOCKED` as an unknown status (and asserts that refusal), and the Jev readiness question's lowest legend is "Blocked or unclear". **No `BLOCKED` status exists anywhere**, in iCalendar, in policy or in a string comparison. No existing test was changed, weakened or deleted. ## Disclosure — choices this PR made where the spec was silent 1. **Request shape.** `cli-api.md` documented `Context add` as `text, optional item_id, reason, supersedes_context_id` and said nothing about blockers. The minimal shape consistent with that body is one new optional member, `blocked_by`, on the existing `item_id` (the blocked item): the link reuses the item the route already resolves live rather than introducing a second item reference or a `kind` discriminator. `docs/technical/cli-api.md` gained the field in that row plus a one-paragraph "Blocker context:" note beside the existing payload paragraphs, in the page's own style. **`docs/` is excluded from this repository's git (`.git/info/exclude`), so that spec edit is local-only and cannot appear in this PR** — it is called out here so the choice is reviewable. 2. **The two ends are validated asymmetrically.** The blocked item must resolve live (it is the id the exclusion matches and the item the memory is tagged to); the blocking item is validated for canonical shape only. Requiring it live would make recording a blocker fail whenever the blocking item has since been deleted or completed, which is exactly when a user is likely to be recording the relationship. 3. **`history_incomplete` is reused** for an unread blocker set. `cli-api.md#error-and-warning-identifiers` is the closed list of non-error warning identifiers and this is the one it documents for "the journal could not be read"; no new identifier was invented. 4. **The planning route (`POST /v1/plans`) reads the same set.** The issue names #24 (`life next`) for the wiring; leaving `plans` on a different input would have made the same policy filter reach two different candidate sets, so the three-line wiring is included and disclosed here as slightly beyond the literal issue text. 5. **Human `life next` prints the exclusion list only when no candidate is eligible** — the renderer's existing policy. The exclusion reason is always in the envelope (`--json`), and `the_human_view_names_the_blocked_exclusion` covers the empty-candidate rendering. 6. **Field validation answers 422 `invalid_input`** (`validation_failed`), matching the sibling body-field validation in the same function (`require_text`) rather than the route's transport-level 400s. ## Review response The first revision of this PR carried a permissive `is_canonical_item_id` (prefix plus "no whitespace"), which accepted `itm_!` and `itm_not-an-id`; the reviewer reproduced both and the PR-body sentence claiming that a "non-canonical `blocked_by`" was refused was therefore false. This revision deletes that helper, promotes the item routes' existing well-formed-id rule so both callers share it, refuses every malformed form at the route (422, no journal row, no outbox row), makes the derivation apply the same rule to a row it reads, and covers each form plus a positive control in `a_blocker_link_needs_both_ends_and_well_formed_item_ids` and `a_malformed_journal_row_never_produces_a_blocker`. The two overstated sentences are rewritten above. ## Defects found outside this PR's scope (reported, not fixed) 1. **`history_unavailable` is emitted but not documented.** `src/api/items.rs:179` and `src/api/context.rs` (four sites) push a `history_unavailable` warning, but `docs/technical/cli-api.md#error-and-warning-identifiers` is the closed list of non-error warning identifiers and does not contain it. This PR deliberately reuses the documented `history_incomplete` rather than adding to that drift, but the existing mismatch is a real one and is left alone here. 2. **One failure code, two HTTP statuses.** `ApiFailure::invalid_input` answers 400 and `ApiFailure::validation_failed` answers 422, and both serialize the error code `invalid_input` (`src/api/mod.rs`). The consequence is visible inside one function: `POST /v1/context` answers 400 for an empty `supersedes_context_id` and 422 for the sibling field validations (`require_text`, and the new `blocked_by` checks). Not touched here — it is a route-wide decision, not this issue's. 3. **`docs/` is not version-controlled in this repository.** `.git/info/exclude` excludes it, so the specification this work was designed against (`docs/technical/*.md`, including the `cli-api.md` paragraph this PR's request shape is documented in) exists only on the working machine and never reaches a branch or a reviewer. The spec edits for this change are therefore local-only; this is called out because it makes "the spec is the source of truth" unenforceable by review as the repository is currently configured. ## Related Issues Closes #63 Refs #22 (the matching half this completes), #18 (the supersession mechanism resolution uses), #24 (the command path the exclusion is observable on) 
`POST /v1/context` accepts an optional `blocked_by`: the canonical
composite item id of the item that blocks the context's `item_id`. The
assertion is recorded as one context fact carrying a structured link
(`after_json.blocker`), by the canonical id the live read established,
and it is refused (422) when the link has one end, is not an `itm_…`
id, or names the item itself.

`history::blockers::active_blockers` derives the set the #22 filter
takes: `context_captured` facts carrying the link, both ends canonical,
minus every fact the journal's supersession ledger records as replaced.
Prose, model output and unresolved external `RELATED-TO` links are not
blocker facts and never enter it.

`life next` and `life plan` read that set into
`FilterContext.active_blockers`, so a blocked task is excluded with
#22's own `blocked` diagnostic and an unread journal is disclosed as
`history_incomplete` rather than read as "nothing is blocked". A
blocker clears by a later fact naming the one it replaces; the journal
stays append-only.

No `BLOCKED` status is introduced anywhere.
yuris changed title from feat(policy): explicit blockers as structured linked context and the derived active set (#63) to feat(policy): explicit blockers as structured linked context and the derived active set 2026-09-23 04:56:32 +00:00
yuris commented 2026-09-23 05:07:44 +00:00
Author
Owner
Copy link

REQUEST CHANGES

Per-criterion review

Criterion Result Evidence
A caller can record a structured blocker fact and its source event is journaled Met src/api/context.rs:307-425 builds the link from the live-resolved blocked item, stores it under after_json.blocker, and returns it. a_blocker_is_recorded_as_a_structured_fact_and_replays_under_its_key verifies the context_captured/user_report row, structured link, outbox row, replay, and conflict behavior. Unknown request fields remain rejected by ContextAddRequest's deny_unknown_fields.
The derived set contains exactly explicit, still-active blockers joined by canonical composite item ID Partial src/history/blockers.rs:54-93 reads only structured context_captured links and removes superseded facts; the blocked end is the live-resolved ApiItem.id, which is the same key consumed at src/domain/policy/next_task_filter.rs:641-643. The blocking end is intentionally allowed to be non-live, as disclosed in the PR body. However, is_canonical_item_id does not actually validate canonical ID shape; see the finding below.
A blocked task is excluded with an observable reason end to end through life next Met a_blocked_task_is_excluded_from_next_with_an_observable_reason exercises the router/filter and observes the existing ExclusionReason::Blocked; the_cli_reports_the_blocked_exclusion_in_its_envelope invokes the real life context add and life next --json binaries; the_human_view_names_the_blocked_exclusion invokes real human-mode life next and observes the task and “blocked by linked context” message. No second exclusion variant was added.
Prose, accepted model output, and unresolved external RELATED-TO never create blockers Met recorded_prose_never_produces_a_blocker, a_model_judgment_never_produces_a_blocker, and an_unresolved_external_related_to_link_never_produces_a_blocker. The model test proves acceptance rather than merely dispatch: it requires basis == "semantic" and non-empty semantic reasons before checking that no blocked exclusion exists.
Explicit resolution clears the blocker append-only and remains explainable Met src/history/blockers.rs:68-90 consults SupersessionLedger; a_resolved_blocker_stops_excluding_the_task_and_is_journaled proves the task becomes eligible, the original structured fact is unchanged, and the later event names the original in supersedes_context_id. SupersessionLedger::supersedes_event remains true for the original even if multiple later facts target it, so ambiguity cannot silently resurrect it; only a new explicit blocker-bearing fact can block again.
No fabricated BLOCKED status Met A case-sensitive search across all of src/ found uppercase BLOCKED only in blocker identifiers/comments and the existing lifecycle rejection test (TaskStatus::parse(Some("BLOCKED")) must fail). No iCalendar status, policy status, or error code was added.
Fingerprinting and idempotency preserve old requests and distinguish blocker bodies Met src/api/context.rs:326-342 starts with the prior semantic object and inserts blocked_by only when stated. The integration test proves same-key/same-body replay and same-key/different-body conflict; comparison with origin/main confirms blocker-free fingerprint input is unchanged.
API contract disclosure Met, with the disclosed repository limitation The local docs/technical/cli-api.md:93,104 contains the minimal request-table addition and blocker paragraph, including resolution and Next/Plan behavior. The PR body accurately discloses that docs/ is excluded and absent from origin/main, so this normative edit is not part of the reviewable commit; skills/life/references/commands.md:69 carries the tracked command reference. This is a real repository-level reviewability limitation, but it is pre-existing and explicitly disclosed rather than hidden by this PR.

Findings

  • major — src/history/blockers.rs:42-45 — Reject malformed blocked_by item IDs. is_canonical_item_id checks only that the value starts with itm_, has one following character, and contains no whitespace. A direct compiled repro returned true for both itm_! and itm_not-an-id, although item_id_for emits uppercase RFC 4648 base32 and the existing item-route validator enforces that alphabet and a minimum length. Because this helper gates both POST /v1/context and journal derivation, such a typo is recorded as a structured fact and then excludes the otherwise valid blocked task until explicit resolution. Correct behavior is a 422 invalid_input: reuse the item route's well-formed-ID rule (or a shared equivalent) and cover punctuation, lowercase/non-base32, and too-short itm_ values.

Verification I ran

  • nix develop --command cargo test — 754 passed, 0 failed, 0 ignored, across 41 Cargo suite-result lines (including the zero-test binary/doc-test suites). tests/issue63_blockers.rs: 11/11 passed.
  • nix develop --command cargo clippy --all-targets -- -D warnings — completed successfully; 0 errors, 0 warnings.
  • nix develop --command cargo fmt --check — completed successfully with no output.
  • Direct compiled validator repro — itm_!: true, itm_not-an-id: true, itm_ABCDEFGH: true; the first two are not IDs the canonical generator can issue.

PR-body claims

  • Reproduced: “754 passed; 0 failed” across 41 suite results, clean Clippy, and clean formatting.
  • Reproduced: all 11 new tests exist in tests/issue63_blockers.rs; no existing test was modified, weakened, or deleted.
  • Reproduced: accepted model output is genuinely used before the no-blocker assertion; CLI tests execute the real binary; resolution leaves both facts in the append-only journal.
  • Reproduced in substance: the full-src/ blocked search contains no fabricated status.
  • Overstated: “an empty or non-canonical blocked_by [is refused]” and the claim that active_blockers requires “both ends canonical.” Prefix-only values such as itm_! are accepted by the added validator and derivation.
  • Accurate disclosure: the local cli-api.md addition exists, and git show origin/main:docs/technical/cli-api.md fails because docs/ is not versioned. The PR body does not conceal that limitation.

Anything I could not check

  • I did not run credential-gated compatibility checks against live Radicale or Hindsight instances. The acceptance behavior here is exercised through deterministic TCP mocks plus the real router, SQLite worker, policy, and CLI; no external backend mutation was required.
REQUEST CHANGES ## Per-criterion review | Criterion | Result | Evidence | |---|---|---| | A caller can record a structured blocker fact and its source event is journaled | Met | `src/api/context.rs:307-425` builds the link from the live-resolved blocked item, stores it under `after_json.blocker`, and returns it. `a_blocker_is_recorded_as_a_structured_fact_and_replays_under_its_key` verifies the `context_captured`/`user_report` row, structured link, outbox row, replay, and conflict behavior. Unknown request fields remain rejected by `ContextAddRequest`'s `deny_unknown_fields`. | | The derived set contains exactly explicit, still-active blockers joined by canonical composite item ID | Partial | `src/history/blockers.rs:54-93` reads only structured `context_captured` links and removes superseded facts; the blocked end is the live-resolved `ApiItem.id`, which is the same key consumed at `src/domain/policy/next_task_filter.rs:641-643`. The blocking end is intentionally allowed to be non-live, as disclosed in the PR body. However, `is_canonical_item_id` does not actually validate canonical ID shape; see the finding below. | | A blocked task is excluded with an observable reason end to end through `life next` | Met | `a_blocked_task_is_excluded_from_next_with_an_observable_reason` exercises the router/filter and observes the existing `ExclusionReason::Blocked`; `the_cli_reports_the_blocked_exclusion_in_its_envelope` invokes the real `life context add` and `life next --json` binaries; `the_human_view_names_the_blocked_exclusion` invokes real human-mode `life next` and observes the task and “blocked by linked context” message. No second exclusion variant was added. | | Prose, accepted model output, and unresolved external `RELATED-TO` never create blockers | Met | `recorded_prose_never_produces_a_blocker`, `a_model_judgment_never_produces_a_blocker`, and `an_unresolved_external_related_to_link_never_produces_a_blocker`. The model test proves acceptance rather than merely dispatch: it requires `basis == "semantic"` and non-empty semantic reasons before checking that no blocked exclusion exists. | | Explicit resolution clears the blocker append-only and remains explainable | Met | `src/history/blockers.rs:68-90` consults `SupersessionLedger`; `a_resolved_blocker_stops_excluding_the_task_and_is_journaled` proves the task becomes eligible, the original structured fact is unchanged, and the later event names the original in `supersedes_context_id`. `SupersessionLedger::supersedes_event` remains true for the original even if multiple later facts target it, so ambiguity cannot silently resurrect it; only a new explicit blocker-bearing fact can block again. | | No fabricated `BLOCKED` status | Met | A case-sensitive search across all of `src/` found uppercase `BLOCKED` only in blocker identifiers/comments and the existing lifecycle rejection test (`TaskStatus::parse(Some("BLOCKED"))` must fail). No iCalendar status, policy status, or error code was added. | | Fingerprinting and idempotency preserve old requests and distinguish blocker bodies | Met | `src/api/context.rs:326-342` starts with the prior semantic object and inserts `blocked_by` only when stated. The integration test proves same-key/same-body replay and same-key/different-body conflict; comparison with `origin/main` confirms blocker-free fingerprint input is unchanged. | | API contract disclosure | Met, with the disclosed repository limitation | The local `docs/technical/cli-api.md:93,104` contains the minimal request-table addition and blocker paragraph, including resolution and Next/Plan behavior. The PR body accurately discloses that `docs/` is excluded and absent from `origin/main`, so this normative edit is not part of the reviewable commit; `skills/life/references/commands.md:69` carries the tracked command reference. This is a real repository-level reviewability limitation, but it is pre-existing and explicitly disclosed rather than hidden by this PR. | ## Findings - **major — `src/history/blockers.rs:42-45` — Reject malformed `blocked_by` item IDs.** `is_canonical_item_id` checks only that the value starts with `itm_`, has one following character, and contains no whitespace. A direct compiled repro returned `true` for both `itm_!` and `itm_not-an-id`, although `item_id_for` emits uppercase RFC 4648 base32 and the existing item-route validator enforces that alphabet and a minimum length. Because this helper gates both `POST /v1/context` and journal derivation, such a typo is recorded as a structured fact and then excludes the otherwise valid blocked task until explicit resolution. Correct behavior is a 422 `invalid_input`: reuse the item route's well-formed-ID rule (or a shared equivalent) and cover punctuation, lowercase/non-base32, and too-short `itm_` values. ## Verification I ran - `nix develop --command cargo test` — **754 passed, 0 failed, 0 ignored**, across **41** Cargo suite-result lines (including the zero-test binary/doc-test suites). `tests/issue63_blockers.rs`: 11/11 passed. - `nix develop --command cargo clippy --all-targets -- -D warnings` — completed successfully; 0 errors, 0 warnings. - `nix develop --command cargo fmt --check` — completed successfully with no output. - Direct compiled validator repro — `itm_!: true`, `itm_not-an-id: true`, `itm_ABCDEFGH: true`; the first two are not IDs the canonical generator can issue. ## PR-body claims - Reproduced: “754 passed; 0 failed” across 41 suite results, clean Clippy, and clean formatting. - Reproduced: all 11 new tests exist in `tests/issue63_blockers.rs`; no existing test was modified, weakened, or deleted. - Reproduced: accepted model output is genuinely used before the no-blocker assertion; CLI tests execute the real binary; resolution leaves both facts in the append-only journal. - Reproduced in substance: the full-`src/` `blocked` search contains no fabricated status. - **Overstated:** “an empty or non-canonical `blocked_by` [is refused]” and the claim that `active_blockers` requires “both ends canonical.” Prefix-only values such as `itm_!` are accepted by the added validator and derivation. - Accurate disclosure: the local `cli-api.md` addition exists, and `git show origin/main:docs/technical/cli-api.md` fails because `docs/` is not versioned. The PR body does not conceal that limitation. ## Anything I could not check - I did not run credential-gated compatibility checks against live Radicale or Hindsight instances. The acceptance behavior here is exercised through deterministic TCP mocks plus the real router, SQLite worker, policy, and CLI; no external backend mutation was required.
`is_canonical_item_id` checked only the `itm_` prefix and whitespace, so
`itm_!` and `itm_not-an-id` were accepted by `POST /v1/context` and
trusted by the derivation: a typo became a structured fact naming an
item nobody can look up.

The item routes already had the rule (`src/api/items.rs`
`is_well_formed_id`: `itm_` plus at least eight RFC 4648 base32
characters). It now lives beside `item_id_for` in `src/api/types.rs`
with the base32 alphabet as one shared const, and the item routes, the
blocker link and `active_blockers` all use it — one rule, not two.

A malformed `blocked_by` is refused with 422 `invalid_input` before any
write (no journal row, no outbox row), and a journal row whose ends are
malformed is ignored by the derivation rather than trusted. Tests cover
each malformed form plus a positive control, and the derivation's
behaviour on a hand-written malformed row.
yuris commented 2026-09-23 05:15:24 +00:00
Author
Owner
Copy link

APPROVE

Follow-up review at head c30c943: the prior malformed-ID finding is fixed.

  • Single shared predicate: src/api/types.rs::is_well_formed_id is now the only item-ID shape rule. It requires itm_ plus at least MIN_ITEM_ID_SUFFIX = 8 characters, and every suffix byte must be in the shared RFC 4648 uppercase base32 BASE32_ALPHABET. item_id_for uses that same alphabet. The item routes, POST /v1/context's blocked_by validation, and src/history/blockers.rs all call this predicate; no second alphabet/shape check or old is_canonical_item_id remains in src/.
  • Malformed input is refused before recording: src/api/context.rs:256 returns 422 invalid_input before item resolution, journal preparation, or commit. a_blocker_link_needs_both_ends_and_well_formed_item_ids covers punctuation, hyphenated, lowercase, empty/short, wrong-prefix and free-text forms, verifies no journal/outbox rows, and uses a generated ID as the positive control.
  • Malformed journal rows are not trusted: src/history/blockers.rs:79-82 applies the same shared predicate to both extracted link ends. a_malformed_journal_row_never_produces_a_blocker inserts a malformed row directly, proves the named task remains a candidate, then proves a well-formed row excludes it.
  • Resolution and observability remain intact: the append-only supersession test, real CLI JSON test, human CLI test, and never-clause tests remain present and pass. The existing ExclusionReason::Blocked variant is still the only exclusion reason.
  • No fabricated status: the PR's updated source grep and lifecycle rejection remain consistent; no STATUS:BLOCKED was introduced.
  • Contract disclosure: the PR body now explicitly records the validator correction and accurately discloses the unversioned docs/ limitation. I have no new finding about that pre-existing repository limitation.

Verification I ran

  • nix develop --command cargo test — 755 passed, 0 failed, 0 ignored, across 41 suite-result lines; tests/issue63_blockers.rs now reports 12 passing tests.
  • nix develop --command cargo clippy --all-targets -- -D warnings — completed successfully, 0 errors and 0 warnings.
  • nix develop --command cargo fmt --check — completed successfully with no output.

The prior request for changes is fully addressed; this PR is ready to merge.

APPROVE Follow-up review at head `c30c943`: the prior malformed-ID finding is fixed. - **Single shared predicate:** `src/api/types.rs::is_well_formed_id` is now the only item-ID shape rule. It requires `itm_` plus at least `MIN_ITEM_ID_SUFFIX = 8` characters, and every suffix byte must be in the shared RFC 4648 uppercase base32 `BASE32_ALPHABET`. `item_id_for` uses that same alphabet. The item routes, `POST /v1/context`'s `blocked_by` validation, and `src/history/blockers.rs` all call this predicate; no second alphabet/shape check or old `is_canonical_item_id` remains in `src/`. - **Malformed input is refused before recording:** `src/api/context.rs:256` returns 422 `invalid_input` before item resolution, journal preparation, or commit. `a_blocker_link_needs_both_ends_and_well_formed_item_ids` covers punctuation, hyphenated, lowercase, empty/short, wrong-prefix and free-text forms, verifies no journal/outbox rows, and uses a generated ID as the positive control. - **Malformed journal rows are not trusted:** `src/history/blockers.rs:79-82` applies the same shared predicate to both extracted link ends. `a_malformed_journal_row_never_produces_a_blocker` inserts a malformed row directly, proves the named task remains a candidate, then proves a well-formed row excludes it. - **Resolution and observability remain intact:** the append-only supersession test, real CLI JSON test, human CLI test, and never-clause tests remain present and pass. The existing `ExclusionReason::Blocked` variant is still the only exclusion reason. - **No fabricated status:** the PR's updated source grep and lifecycle rejection remain consistent; no `STATUS:BLOCKED` was introduced. - **Contract disclosure:** the PR body now explicitly records the validator correction and accurately discloses the unversioned `docs/` limitation. I have no new finding about that pre-existing repository limitation. ## Verification I ran - `nix develop --command cargo test` — **755 passed, 0 failed, 0 ignored**, across **41** suite-result lines; `tests/issue63_blockers.rs` now reports 12 passing tests. - `nix develop --command cargo clippy --all-targets -- -D warnings` — completed successfully, 0 errors and 0 warnings. - `nix develop --command cargo fmt --check` — completed successfully with no output. The prior request for changes is fully addressed; this PR is ready to merge.
yuris merged commit a39377e885 into main 2026-09-23 05:15:46 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/life-service!79
No description provided.