Fix recoverable attention lifecycle and generic foreground integration - #73
Fix recoverable attention lifecycle and generic foreground integration#73TKCen wants to merge 8 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a70f1906e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "source_binding": binding, | ||
| } | ||
| ) | ||
| payload = json.dumps(items, ensure_ascii=False, sort_keys=True, separators=(",", ":")) |
There was a problem hiding this comment.
When a legacy, corrupt, or externally produced advisory contains a very long task identifier or large event_ids/source_candidate_ids lists, this serializes those values directly into every pre-LLM context without a per-field or total-size limit. The aperture item count is bounded, but the prompt is not, so enabling the doorway can unexpectedly consume the model context window or prevent the foreground turn from completing; cap identifiers and list lengths and enforce a maximum serialized payload size before returning the hook context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Bounded display fields, list lengths, serialized payload and total context bytes; authority tokens are rejected rather than truncated. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
| rewritten = [ | ||
| preview_rows.get(str(candidate.get("id") or ""), candidate) | ||
| for candidate in candidates | ||
| ] | ||
| store.rewrite_jsonl("candidates", rewritten) |
There was a problem hiding this comment.
When the enabled foreground doorway claims an item concurrently with a sensor ingest or Kanban/attention settlement, this whole-file rewrite can overwrite the other process's update because the new aperture lock is used only by conscious_aperture.py; writers such as _rewrite_candidates in settlement.py and candidate coalescing/appends in tools.py do not acquire it. Since the hook now performs this rewrite during ordinary foreground turns while scheduled writers may run, an interleaving read followed by this replace can delete a newly appended candidate or resurrect a candidate another writer just settled, so candidate mutations need a shared profile-wide lock or compare-and-swap discipline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Candidate writers now share SensoriumStore.candidate_transaction(), including append/coalescing and settlement. Multiprocess interleaving regressions cover the shared boundary. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
| if keyword in settlement_decisions and ( | ||
| aperture_id or consumer_id or current_lease | ||
| ): |
There was a problem hiding this comment.
When a normal cand_... item is currently leased, only the four keywords in settlement_decisions enter the ownership-checked path; suppress and cancel instead fall through to handle_sensorium_candidate_update, which accepts both actions and rewrites the candidate without checking aperture_id or consumer_id. Any foreground caller that knows the candidate id can therefore remove an unresolved leased item from the aperture without owning it and without a conscious.aperture.settled receipt, so all mutations of in_conscious_aperture candidates must be rejected or routed through exact ownership validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Generic updates, Kanban settlement and compaction reject leased-row mutation; exact-token aperture settlement remains authoritative. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
| "settle": "SETTLED", | ||
| "hold": "HELD", | ||
| } | ||
| store = SensoriumStore(instance=instance) |
There was a problem hiding this comment.
When a host invokes the new pre-LLM hook with a state_dir override, the doorway leases candidates in that directory, but the live update path constructs this store from only the instance and ignores the handler's kw.get("state_dir"). Even if the subsequent tool call carries the same override, settlement reads the default profile instead and reports the candidate missing or unleased, leaving the actual item stuck until lease expiry; construct the settlement store with the same state-directory override forwarded to the hook.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Live settlement and generic update paths now propagate the state_dir override. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d21fe1d805
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| surface = str(platform or "local") | ||
| if not doorway_config["enabled"] or surface not in doorway_config["surfaces"]: |
There was a problem hiding this comment.
When an instance and candidate both allow discord and conscious_doorway.surfaces includes it, this check enables the hook and injects the complete advisory packet into a Discord foreground turn. That makes the new doorway configurable beyond the MVP's required local-only boundary; reject non-local surfaces here rather than relying solely on operator configuration.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. The doorway enforces the local surface even when remote surfaces are configured. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
| "opened_at": now_iso, | ||
| "state": "open", | ||
| "consumer_id": owner, | ||
| "lease_expires_at": lease_expires_at, |
There was a problem hiding this comment.
When lease_minutes is shorter than 180 minutes (including the new 15-minute default), a candidate becomes reclaimable at this explicit timestamp, but _derived_stale_aperture_ids in settlement.py and _candidate_liveness in dashboard/plugin_api.py still calculate staleness solely from opened_at + 180m. During that gap the aperture logic permits another consumer to reclaim the item while both liveness surfaces incorrectly report it as actively reviewing and non-actionable, so those projections should prefer lease_expires_at and use the legacy timeout only when it is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Both settlement and dashboard liveness projections prefer explicit lease_expires_at with legacy fallback. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
| for candidate in candidates: | ||
| candidate_id = str(candidate.get("id") or "") | ||
| if canonical_ids.get(_logical_source_key(candidate)) != candidate_id: | ||
| continue | ||
| if not _visible(candidate, surface=surface, instance_config=instance_config): |
There was a problem hiding this comment.
When the JSONL store contains a legacy or externally produced candidate with a truthy non-dictionary conscious_task or advisory_meta, this unconditional _logical_source_key call reaches _source_binding and raises AttributeError before the later eligibility checks. The foreground doorway catches that exception and silently returns no context on every turn, while direct aperture callers crash, so one malformed unrelated row can indefinitely prevent all valid advisory candidates from being leased; skip non-advisory/non-dictionary rows before computing the key or make _source_binding normalize these fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8b63f7a. Malformed metadata rows are excluded before source binding so valid neighboring candidates remain eligible. Independent bounded review verified this finding closed; exact-head CI passed on Python 3.11 and 3.12. Full disposable-copy suite: 1439 passed, 1 skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b63f7af67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| PROFILE = os.environ.get("SENSORIUM_SUBCONSCIOUS_PROFILE", "serasubconscious") | ||
| # Deployments inject the Hermes reviewer profile through configuration or the | ||
| # environment; the reusable fallback contains no installation-specific identity. | ||
| PROFILE = os.environ.get("SENSORIUM_SUBCONSCIOUS_PROFILE", "subconscious-reviewer") |
There was a problem hiding this comment.
When SENSORIUM_SUBCONSCIOUS_PROFILE is unset, this now assigns every generated review task to the literal placeholder subconscious-reviewer. The documented migration path is to inject the real subconscious_profile through instance.config.json, but this script never loads that configuration, so existing deployments that relied on the previous working default—or follow the new configuration guidance—will leave their blocked intake tasks assigned to a nonexistent reviewer lane. Load the selected instance's configured profile here while retaining the environment variable as an override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 34d6802. Reviewer selection now resolves after final instance selection, using environment override, validated instance configuration, then generic fallback across all callers. Independent delta closure approved; Python 3.11 and 3.12 CI passed on this exact head.
| recovery_eligible, | ||
| fresh_eligible, | ||
| limit=min(remaining_size, available_capacity), | ||
| last_lane=_last_fairness_lane(store.read_jsonl("decisions")), |
There was a problem hiding this comment.
When an enabled profile has accumulated a large decisions.jsonl, every foreground pre-LLM claim parses the entire audit log here just to find the latest aperture lane; the subsequent presentation-attempt path performs another full decisions scan. Because SensoriumStore.read_jsonl() reads and decodes every physical line even when given a limit, foreground latency and memory use grow with the profile's lifetime while the candidate lock is held, also blocking scheduled candidate writers. Read the log from the tail or persist the last-served lane separately so this hot path remains bounded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 34d6802. Foreground fairness and presentation idempotency use bounded metadata instead of lifetime audit scans. Missing metadata with active leases and corrupt metadata fail closed; virgin initialization and expired recovery remain supported. Independent delta closure approved; Python 3.11 and 3.12 CI passed on this exact head.
| if explicit_consumer | ||
| else [] | ||
| ) | ||
| resumable = sorted(resumable, key=_candidate_sort_key)[:size] |
There was a problem hiding this comment.
When the same foreground session resumes an item shortly before its existing lease expires, this accepts it solely because the expiry is still in the future and returns the old lease_expires_at unchanged. For example, an item resumed one second before expiry is injected into the model, but by the time the model issues its settlement tool call _validate_current_ownership rejects it as expired, so a valid conscious decision cannot be recorded. Renew the lease on resumption, or treat leases without enough remaining processing time as expired and reclaim them with a fresh interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 34d6802. Same-owner resumption atomically renews the lease without shortening existing expiry, preserving exact ownership and dry-run immutability. Independent delta closure approved; Python 3.11 and 3.12 CI passed on this exact head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d6802bf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if requires_exact_settlement(target): | ||
| return _err(instance, "candidate_leased_requires_exact_settlement") |
There was a problem hiding this comment.
When an aperture item is settled as HELD with a future return_at, its status is no longer in_conscious_aperture, so this guard does not protect it and the live keyword="resume" path immediately changes it back to candidate. The next doorway call can then reclaim it before held_return.not_before (while also leaving the stale checkpoint metadata attached), defeating the new time-checkpoint lifecycle; reject generic resume for checkpoint-held aperture rows until they are due, or route it through a checkpoint-aware transition.
Useful? React with 👍 / 👎.
| lease_expiry = _parse_iso(current.get("lease_expires_at")) | ||
| if lease_expiry is not None and now_dt >= lease_expiry: |
There was a problem hiding this comment.
When an exact-owned migrated or repaired lease lacks a valid lease_expires_at, _parse_iso returns None and this check permits settlement indefinitely, even after open_conscious_aperture considers the same row expired using opened_at + stale_after_minutes. The previous owner can therefore settle an otherwise reclaimable item until another consumer wins the reclaim race; settlement should use the same legacy expiry fallback and fail closed on a malformed explicit expiry.
Useful? React with 👍 / 👎.
| process_lock = _candidate_process_lock(key) | ||
| with process_lock: | ||
| held = getattr(_HELD_CANDIDATE_LOCKS, "locks", None) |
There was a problem hiding this comment.
When two threads each hold a candidate transaction for a different profile and then enter the other profile, each inner call blocks on the other thread's process_lock here before reaching the intended cross-root RuntimeError check. This creates the exact AB/BA deadlock the nesting guard is meant to prevent; inspect the thread-local held-root state and reject a different key before acquiring its process lock.
Useful? React with 👍 / 👎.
1 participant
Summary
Replace global stale-attention blocking with bounded per-item leases, exact owner/source checks, resumable attention, and deterministic fairness between returning and fresh items. Add an opt-in local foreground doorway and executable hold checkpoints. Presentation attempts are not claimed as model consumption.
Boundaries and repair
Verification
Current candidate:
8b63f7af672a00c4a82358c0e3acb7403878b145.Runtime gate
Public CI and local runtime adoption remain separate gates. Final composed plugin bytes must pass isolated request-assembly verification and deployment checks before claiming live delivery. A hook attempt receipt never proves model consumption. No live deployment is included in this PR.