Repository navigation
Harden offline incidents and virtual eIM transitions - #1
Merged
Merged
Conversation
Architecture 
Inside the changed components — 2 viewsComponent view — Recovery Agent Runtime Model selection and tool wiring inside the recovery agent container. 
Component view — Virtual eIM & Twin State transitions, target validation, and eIM execution logic. 
Data flow 
The other flows — 1 sequence
Review ✨ Nothing flagged · reviewed Tip Set 🪧 More tips
Thanks for using PR Lens! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out on X, Bluesky, Mastodon, Reddit or LinkedIn |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_agent.py">
<violation number="1" location="tests/test_agent.py:75">
P3: This test does not verify caching: the constant lambda returns the same object on every call, and the test constructs only one incident. Track factory calls across multiple enabled incidents and assert one cached result is reused, otherwise a regression to per-incident Bedrock construction will pass.</violation>
</file>
<file name="docs/architecture.md">
<violation number="1" location="docs/architecture.md:102">
P3: The documentation overstates the uncached behavior: `enable_profile` caches the `IPA unreachable` refusal, so repeating that approval emits no new audit event or simulated time. Distinguish cached delivery failures from the uncached validation, approval, capability, and owner refusals.</violation>
</file>
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
| passed_to_tools.append(llm) | ||
| return real_build_tools(twin, approver, llm) | ||
|
|
||
| monkeypatch.setattr(ag, "bedrock_model", lambda: sentinel) |
There was a problem hiding this comment.
P3: This test does not verify caching: the constant lambda returns the same object on every call, and the test constructs only one incident. Track factory calls across multiple enabled incidents and assert one cached result is reused, otherwise a regression to per-incident Bedrock construction will pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/test_agent.py, line 75:
<comment>This test does not verify caching: the constant lambda returns the same object on every call, and the test constructs only one incident. Track factory calls across multiple enabled incidents and assert one cached result is reused, otherwise a regression to per-incident Bedrock construction will pass.</comment>
<file context>
@@ -22,6 +25,59 @@ def choose(self, state, instructions, options, name="next_tool"):
+ passed_to_tools.append(llm)
+ return real_build_tools(twin, approver, llm)
+
+ monkeypatch.setattr(ag, "bedrock_model", lambda: sentinel)
+ monkeypatch.setattr(ag, "build_tools", capture_tools)
+ inc = ag.Incident("TRANSIENT", decider=ScriptedDecider(["retry_connection"]), use_bedrock=True)
</file context>
Comment on lines
+102
to
+103
| state change. New invalid-target and other command-not-sent refusals remain uncached audit events; under the | ||
| simulation's existing event accounting, each `eim_enable_profile` refusal still represents 30 simulated minutes. |
There was a problem hiding this comment.
P3: The documentation overstates the uncached behavior: enable_profile caches the IPA unreachable refusal, so repeating that approval emits no new audit event or simulated time. Distinguish cached delivery failures from the uncached validation, approval, capability, and owner refusals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/architecture.md, line 102:
<comment>The documentation overstates the uncached behavior: `enable_profile` caches the `IPA unreachable` refusal, so repeating that approval emits no new audit event or simulated time. Distinguish cached delivery failures from the uncached validation, approval, capability, and owner refusals.</comment>
<file context>
@@ -96,8 +96,12 @@ What is modelled, what is simplified and what is left out, in telecom terms, is
+ stands in for SGP.32's IPA-triggered rollback). The virtual eIM requires a string target naming an installed,
+ inactive profile before it attempts delivery. Exact duplicate commands are served from the approval-keyed cache
+ first—even when the successful target is now active—so they return the original result without another event or
+ state change. New invalid-target and other command-not-sent refusals remain uncached audit events; under the
+ simulation's existing event accounting, each `eim_enable_profile` refusal still represents 30 simulated minutes.
+ No profile download, no cryptography, no certified SGP.32 flow.
</file context>
Suggested change
| state change. New invalid-target and other command-not-sent refusals remain uncached audit events; under the | |
| simulation's existing event accounting, each `eim_enable_profile` refusal still represents 30 simulated minutes. | |
| New invalid-target, approval, capability and owner refusals remain uncached audit events; an IPA-unreachable result is cached under its approval, so repeating it emits no new event. | |
| The first event for each `eim_enable_profile` refusal still represents 30 simulated minutes. |
… names The eIM cache is keyed by approval alone, so the docs no longer call cached results exact duplicates. Drop the duplicated ordering prose and em dashes, rename the model-factory test (it does not test caching) and drop an assertion that could never fail. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/architecture.md">
<violation number="1" location="docs/architecture.md:125">
P3: Non-string targets raise `TypeError` rather than producing a refusal, so this sentence should say so. Add that case to the refusal list.</violation>
</file>
<file name=".claude/skills/verify/SKILL.md">
<violation number="1" location=".claude/skills/verify/SKILL.md:26">
P3: The gotcha overstates replay's AWS independence: `--replay` alone still constructs `BedrockModel` unless `--no-bedrock` is also supplied. Qualify this as `make replay` or state that direct replays require `--no-bedrock`, otherwise a verifier with an invalid `AWS_PROFILE` can hit `ProfileNotFound`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
| for an eUICC that cannot enable profiles or a fleet whose owner forbids switching, and one it cannot deliver because | ||
| the management path is down. It does not repeat the gate's evidence checks (reject cause, reselection, coverage). A test | ||
| replaces the gate with one that always passes and checks that the eIM still refuses an unapproved switch. | ||
| Independently of the gate, the virtual eIM refuses an enable command whose target is not an installed, inactive |
There was a problem hiding this comment.
P3: Non-string targets raise TypeError rather than producing a refusal, so this sentence should say so. Add that case to the refusal list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/architecture.md, line 125:
<comment>Non-string targets raise `TypeError` rather than producing a refusal, so this sentence should say so. Add that case to the refusal list.</comment>
<file context>
@@ -126,13 +122,12 @@ Decider's options beforehand:
-whose owner forbids switching, and one it cannot deliver because the management path is down. It does not repeat the
-gate's evidence checks (reject cause, reselection, coverage). A test replaces the gate with one that always passes and
-checks that the eIM still refuses an unapproved switch.
+Independently of the gate, the virtual eIM refuses an enable command whose target is not an installed, inactive
+profile, whose approval does not match the proposal, for an eUICC that cannot enable profiles or a fleet whose owner
+forbids switching, and one it cannot deliver because the management path is down. A command that reuses an approval
</file context>
| ## Gotchas | ||
|
|
||
| - An invalid `AWS_PROFILE` name crashes even `--no-bedrock` runs: the inner Strands agent always builds a BedrockModel. | ||
| - `--no-bedrock` runs, replay and validate ignore `AWS_PROFILE`. `make test` still needs it unset or valid: the pattern |
There was a problem hiding this comment.
P3: The gotcha overstates replay's AWS independence: --replay alone still constructs BedrockModel unless --no-bedrock is also supplied. Qualify this as make replay or state that direct replays require --no-bedrock, otherwise a verifier with an invalid AWS_PROFILE can hit ProfileNotFound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .claude/skills/verify/SKILL.md, line 26:
<comment>The gotcha overstates replay's AWS independence: `--replay` alone still constructs `BedrockModel` unless `--no-bedrock` is also supplied. Qualify this as `make replay` or state that direct replays require `--no-bedrock`, otherwise a verifier with an invalid `AWS_PROFILE` can hit `ProfileNotFound`.</comment>
<file context>
@@ -23,5 +23,6 @@ README "Live" section) and, for Bedrock, `AWS_PROFILE=<your-profile>`.
## Gotchas
-- Live Bedrock flows require a valid AWS profile and region configuration.
+- `--no-bedrock` runs, replay and validate ignore `AWS_PROFILE`. `make test` still needs it unset or valid: the pattern
+ tests and the Bedrock-mocked agent tests construct a `BedrockModel`.
- Benchmark runs take ~20-40 min with both Bedrock choosers; run them in the background.
</file context>
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.
Summary
This change hardens two reliability boundaries:
--no-bedrockincidents now receive a private provider-neutral StrandsModel. Offline replay, trace validation, benchmark helpers, and replay-backed UI flows no longer initialize AWS. Accidental inference fails closed with the fixed sanitized errorinference is disabled for direct-tool-only incidents.FleetTwin.enable_profilenow validates uncached targets before proposal checks or operational mutation. Non-string targets raiseTypeError("target must be a string"); unknown and already-active strings produce uncached, audit-visible refusals. Approval-keyed duplicate commands remain cache-first and return the identical result object without new events, profile changes, or simulated time.The demonstrated benefits are reliability, dependency isolation, and atomic state handling, not measured performance. No comparable raw before/after performance evidence was collected, so this PR makes no performance-improvement claim.
Reproduced failures and fixes
env AWS_PROFILE=simshift-profile-that-does-not-exist DECIDER_URL=http://127.0.0.1:1 .venv/bin/python -m simshift.agent TRANSIENT --approve --no-bedrock --replay docs/sample-runs/TRANSIENT.jsonexited 1 with
botocore.exceptions.ProfileNotFoundbecauseIncidentalways constructed a Bedrock model. Offline construction now uses_DirectToolModel; the enabled branch still obtains and retains the exact cached realBedrockModel.FleetTwin("BR_PROFILE").enable_profile("profile_C", proposal_id)raisedKeyErrorafter changingactive_profileto an uninstalled profile and emitted no audit event. Uncached malformed, unknown, and active targets are now rejected before mutation. Unknown/active string refusals retain the existing single-event, 30-minute accounting and remain uncached.Compatibility boundaries
The change preserves Python
>=3.12, Strands Agents 1.61.1 and its provider-neutralModelinterface, Pydantic 2, the standard-library loopback web stack, public module/CLI commands, enabled Bedrock model ID/region/token settings, advisory plan review and explanations, orchestrator behavior, approval/proposal/capability/owner/reachability precedence for valid targets, rollback behavior, routing prompts and thresholds, policy rules, six scenario outcomes, trace schema and recordings, benchmark records, HTTP/JavaScript contracts, dependencies, packaging configuration, and frontend behavior.Published traces and
docs/benchmark/benchmark-2026-10-19.jsonhave no branch or working-tree changes. The complete diff contains only the seven planned source, test, and documentation files; inspection found no credentials, generated artifacts, cache files, or unrelated churn.Verification evidence
Focused and full tests
All 12 published trace validations
All 12/12 validations reported
ok:docs/sample-runs/BR_PROFILE.jsondocs/sample-runs/DE_OPERATOR.jsondocs/sample-runs/FW_LOOP.jsondocs/sample-runs/JP_INGEST.jsondocs/sample-runs/OFFLINE.jsondocs/sample-runs/TRANSIENT.jsondocs/sample-runs/orchestrated/BR_PROFILE.jsondocs/sample-runs/orchestrated/DE_OPERATOR.jsondocs/sample-runs/orchestrated/FW_LOOP.jsondocs/sample-runs/orchestrated/JP_INGEST.jsondocs/sample-runs/orchestrated/OFFLINE.jsondocs/sample-runs/orchestrated/TRANSIENT.jsonSix deterministic replay routes and outcomes
BR_PROFILE:gather_evidence -> query_apn_session_app_diagnostics;gather_evidence -> query_firmware_modem_diagnostics;act -> prepare_guarded_profile_transition; restored=True.DE_OPERATOR:gather_evidence -> query_apn_session_app_diagnostics;act -> simulate_operator_reselection; restored=True.FW_LOOP:gather_evidence -> query_firmware_modem_diagnostics;gather_evidence -> query_apn_session_app_diagnostics;fallback -> defer_or_escalate; restored=False, safely escalated.JP_INGEST:act -> query_apn_session_app_diagnostics;gather_evidence -> query_firmware_modem_diagnostics;fallback -> defer_or_escalate; restored=False, safely escalated.OFFLINE:gather_evidence -> query_apn_session_app_diagnostics;fallback -> defer_or_escalate; restored=False, safely escalated.TRANSIENT:act -> simulate_retry_with_backoff; restored=True.CLI, HTTP, JavaScript, and distribution checks
AWS_PROFILE=simshift-profile-that-does-not-exist, the documentedTRANSIENT --approve --no-bedrock --replaycommand exited 0, identifiedno Bedrock, routed tosimulate_retry_with_backoff, ended withrestored=True, and contained noProfileNotFound.BR_PROFILEto proposal-bound human approval, rejected another step with HTTP 409 while pending, accepted literal JSONtrue, and completed in restoredSUCCEEDEDstate.node --check simshift/static/app.jsexited 0.uv buildproduced exactly 1 sdist and 1 wheel. Wheel assertions found all 3/3 required assets:simshift/static/app.js,simshift/static/index.html, andsimshift/static/style.css.simshift.staticis absent from explicit package configuration was observed and intentionally not changed.origin/main...HEADgit diff --checkcommands exited 0; immutable fixture/benchmark diff checks also exited 0.pyproject.toml,Makefile, and CI configure no lint command, formatter command, or static type-check command. These checks are recorded as absent rather than misrepresented as passing; Python tests/import execution, JavaScript parsing, package build, and whitespace checks are separate evidence.No live Decider/Bedrock benchmark was run. The measured facts above support dependency isolation and state-boundary reliability; any future latency or recovery-quality claim requires comparable raw before/after measurements.
Deferred recommendations
simshift.staticdiscovery and add artifact-content CI. Impact: medium packaging reliability. Evidence: the wheel currently contains all static assets, but setuptools emits an ambiguous package-discovery warning and CI does not build artifacts. Tradeoff: package-data/discovery policy must be chosen deliberately to avoid including unwanted packages or tests.>=3.12, CI runs only 3.13, while local 3.12.13 and 3.13.13 both passed 146 tests with one skip. Tradeoff: a version matrix increases CI time and unpinned resolution may differ by interpreter.start()checks but does not setbusy, controls are not uniformly disabled, and snapshots do not take the incident lock. Tradeoff: naive locking can deadlock and stale-response handling needs dedicated browser/concurrency tests.KeyError, zero samples fail late, and--stampis joined into an output path. Tradeoff: this trusted local CLI work is unrelated to the fixed runtime boundaries.