Add orchestration drift guardrail - #15
Open
Pro777 wants to merge 6 commits into
Open
Conversation
WalkthroughAdds Talos data models (TalosPolicy, TalosSnapshot, TalosState, TalosEvaluation), a new runtime module for snapshot evaluation and JSON/JSONL persistence (cycles, status, history, policy), refactors scorer.py to pure policy-driven scoring, adds CLI evaluate/status/explain subcommands, expands package exports, and updates tests and documentation accordingly. ChangesSnapshot Evaluation Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Runtime
participant Scorer
participant FileSystem
User->>CLI: talos evaluate --snapshot snapshot.json
CLI->>Runtime: read_snapshot(path)
CLI->>Runtime: load_state(), load_policy()
CLI->>Runtime: evaluate_snapshot(snapshot, prior_state, policy)
Runtime->>Scorer: compute_talos_level(metrics, policy)
Scorer-->>Runtime: level
Runtime->>Runtime: detect_guardrails(snapshot)
Runtime->>Runtime: explain_level(snapshot, level, guardrails)
Runtime-->>CLI: TalosEvaluation
CLI->>Runtime: save_state(), save_status(), append_history()
Runtime->>FileSystem: write cycles.json, status.json, history.jsonl
CLI-->>User: print evaluation.to_dict()
User->>CLI: talos status
CLI->>FileSystem: read status.json
CLI-->>User: print status JSON
User->>CLI: talos explain --snapshot snapshot.json
CLI->>Runtime: evaluate_snapshot(snapshot)
Runtime-->>CLI: TalosEvaluation.reasons
CLI-->>User: print reasons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
talos/models.py (2)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the dataclass defaults inside
TalosPolicy.from_dict.
TalosPolicy.from_dict()duplicates0.8and2, whiletalos/scorer.pyderivesDEFAULT_POLICYfromTalosPolicy(). If these field defaults change,TalosPolicy()andTalosPolicy.from_dict({})will silently diverge and produce different scoring depending on construction path.Suggested change
`@classmethod` def from_dict(cls, payload: Dict[str, Any]) -> "TalosPolicy": + defaults = cls() return cls( - friction_ratio=float(payload.get("friction_ratio", 0.8)), - write_block_cycles=int(payload.get("write_block_cycles", 2)), + friction_ratio=float(payload.get("friction_ratio", defaults.friction_ratio)), + write_block_cycles=int(payload.get("write_block_cycles", defaults.write_block_cycles)), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@talos/models.py` around lines 16 - 21, `TalosPolicy.from_dict` is hardcoding default values that duplicate the dataclass field defaults, which can drift from the defaults used by `TalosPolicy()` and `DEFAULT_POLICY` in `talos/scorer.py`. Update `TalosPolicy.from_dict` to reuse the class/dataclass defaults for `friction_ratio` and `write_block_cycles` instead of literal values, so empty payloads build the same policy as direct construction. Use the `TalosPolicy` class and its `from_dict` method as the lookup points for the fix.
88-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd
TalosEvaluation.from_dict()to complete the model contract.This is the only Talos model that cannot round-trip through the typed API, even though runtime persists evaluations as structured JSON. That asymmetry pushes any future reader of
status.jsonorhistory.jsonlback to raw dict parsing instead of the shared model contract.Suggested change
`@dataclass` class TalosEvaluation: """Structured output contract for a Talos evaluation cycle.""" @@ source: str = "manual" timestamp: str = "" + `@classmethod` + def from_dict(cls, payload: Dict[str, Any]) -> "TalosEvaluation": + return cls( + level=int(payload["level"]), + previous_level=int(payload["previous_level"]), + cycles_at_level=int(payload["cycles_at_level"]), + wip_total=int(payload["wip_total"]), + global_max=int(payload["global_max"]), + backlog_total=int(payload["backlog_total"]), + backlog_delta=int(payload["backlog_delta"]), + at_cap_projects=list(payload.get("at_cap_projects", [])), + reasons=list(payload.get("reasons", [])), + guardrails=list(payload.get("guardrails", [])), + source=str(payload.get("source", "manual")), + timestamp=str(payload.get("timestamp", "")), + ) + def to_dict(self) -> Dict[str, Any]: return asdict(self)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@talos/models.py` around lines 88 - 106, Add a TalosEvaluation.from_dict() constructor to complete the model contract so persisted evaluation JSON can round-trip through the typed API. Implement it as the inverse of TalosEvaluation.to_dict(), using the TalosEvaluation dataclass fields to build an instance from a dict while preserving defaults like guardrails, source, and timestamp when missing. Keep the parsing logic alongside to_dict() in TalosEvaluation so future readers of status.json or history.jsonl can use the shared model instead of raw dict handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@talos/cli.py`:
- Around line 72-73: The evaluate/explain flow is letting snapshot loading
failures escape as raw tracebacks instead of CLI usage errors. Update the
command handlers that call read_snapshot(), especially the evaluate and explain
paths that use evaluate_snapshot() and any similar snapshot loading logic, to
catch file/JSON/schema/coercion exceptions and route them through
parser.error(...) like the status command. Keep the existing behavior for valid
snapshots, but ensure bad input is reported consistently via the CLI parser.
- Around line 75-84: The persistence check in evaluate is incorrectly using
TALOS_CYCLES_FILE/TALOS_STATUS_FILE/TALOS_HISTORY_FILE existence to infer write
success, so stale files can hide failed overwrites. Update the talos/runtime.py
helpers save_state, save_status, and append_history to either raise on write
failure or return explicit success flags, then change talos/cli.py evaluate to
derive persistence_failed and the exit code from those helper results instead of
probing file existence after the writes.
In `@talos/runtime.py`:
- Around line 51-52: The drift guard in the orchestration check is firing too
early because `intent` and `main_thread_role` are being used as the primary
trigger. Update the logic around the drift check to require a real subagent
handoff by keying off `subagents_requested` and `subagents_spawned`, and use
`intent`/`main_thread_role` only as supporting context. Make sure the same
gating is applied consistently in the related drift handling paths in
`talos.runtime` so a single-thread run cannot report that subagents were
requested unless they actually were.
- Around line 157-158: The zero-cap early return in the explain path skips the
later guardrails handling, so `evaluation.reasons` can miss a message that is
present in `evaluation.guardrails`. Update the relevant flow in `talos.runtime`
(the branch that checks `snapshot.global_max <= 0`, and the similar guardrails
logic later in the same path) so the same zero-cap message is appended to
reasons before returning, or otherwise ensure the guardrails append runs for
this edge case without changing the clean-level behavior.
- Around line 156-164: Keep explain_level() consistent with
compute_talos_level() by using the same friction threshold calculation instead
of flooring it with int(...). Update the threshold check in runtime.py so the
comparison against snapshot.wip_total uses the same float-based global_max *
active_policy.friction_ratio logic as the scorer, while preserving the early
return for snapshot.global_max <= 0. This will keep the reasons generated by
explain_level() aligned with compute_talos_level() and the CLI explain output.
---
Nitpick comments:
In `@talos/models.py`:
- Around line 16-21: `TalosPolicy.from_dict` is hardcoding default values that
duplicate the dataclass field defaults, which can drift from the defaults used
by `TalosPolicy()` and `DEFAULT_POLICY` in `talos/scorer.py`. Update
`TalosPolicy.from_dict` to reuse the class/dataclass defaults for
`friction_ratio` and `write_block_cycles` instead of literal values, so empty
payloads build the same policy as direct construction. Use the `TalosPolicy`
class and its `from_dict` method as the lookup points for the fix.
- Around line 88-106: Add a TalosEvaluation.from_dict() constructor to complete
the model contract so persisted evaluation JSON can round-trip through the typed
API. Implement it as the inverse of TalosEvaluation.to_dict(), using the
TalosEvaluation dataclass fields to build an instance from a dict while
preserving defaults like guardrails, source, and timestamp when missing. Keep
the parsing logic alongside to_dict() in TalosEvaluation so future readers of
status.json or history.jsonl can use the shared model instead of raw dict
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef3b87ba-ef50-46c0-896f-f77d0ca58689
📒 Files selected for processing (13)
README.mdTESTING.mddocs/OVERVIEW.mdtalos/__init__.pytalos/cli.pytalos/models.pytalos/runtime.pytalos/scorer.pytests/test_cli.pytests/test_cycles.pytests/test_models.pytests/test_runtime.pytests/test_scorer.py
Comment on lines
+72
to
+73
| snapshot = read_snapshot(args.snapshot) | ||
| evaluation = evaluate_snapshot(snapshot, prior_state=load_state(), policy=load_policy()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle invalid snapshots as CLI errors.
read_snapshot() can fail on missing files, invalid JSON, or schema coercion errors from TalosSnapshot.from_dict(), but evaluate and explain currently surface those as raw tracebacks. These paths should go through parser.error(...), like status, so bad user input fails predictably.
Also applies to: 97-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@talos/cli.py` around lines 72 - 73, The evaluate/explain flow is letting
snapshot loading failures escape as raw tracebacks instead of CLI usage errors.
Update the command handlers that call read_snapshot(), especially the evaluate
and explain paths that use evaluate_snapshot() and any similar snapshot loading
logic, to catch file/JSON/schema/coercion exceptions and route them through
parser.error(...) like the status command. Keep the existing behavior for valid
snapshots, but ensure bad input is reported consistently via the CLI parser.
Comment on lines
+75
to
+84
| save_state(level=next_state.level, count=next_state.count, last_backlog=next_state.last_backlog) | ||
| save_status(evaluation) | ||
| append_history(evaluation) | ||
| persistence_failed = not ( | ||
| TALOS_CYCLES_FILE.exists() and TALOS_STATUS_FILE.exists() and TALOS_HISTORY_FILE.exists() | ||
| ) | ||
| _print_json(evaluation.to_dict()) | ||
| if persistence_failed: | ||
| logger.error("Talos evaluation completed but one or more persistence writes failed") | ||
| return 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't infer write success from file existence.
save_state, save_status, and append_history each swallow their own write exceptions in talos/runtime.py, so this only catches the “file was never created” case. If an older cycles.json / status.json / history.jsonl already exists, a failed overwrite still leaves .exists() true and evaluate returns 0 with stale persisted state/status. Have the runtime helpers raise or return success flags, then derive the exit code from those results instead of a post-write existence probe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@talos/cli.py` around lines 75 - 84, The persistence check in evaluate is
incorrectly using TALOS_CYCLES_FILE/TALOS_STATUS_FILE/TALOS_HISTORY_FILE
existence to infer write success, so stale files can hide failed overwrites.
Update the talos/runtime.py helpers save_state, save_status, and append_history
to either raise on write failure or return explicit success flags, then change
talos/cli.py evaluate to derive persistence_failed and the exit code from those
helper results instead of probing file existence after the writes.
Comment on lines
+51
to
+52
| intent = str(orchestration.get("intent", orchestration.get("mode", ""))).strip().lower() | ||
| main_thread_role = str(orchestration.get("main_thread_role", "")).strip().lower() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a real subagent handoff before flagging drift.
intent / main_thread_role alone currently trigger the guardrail, so a single-thread run can emit "subagents were requested" even when no subagent was ever requested or spawned. Gate this on subagents_requested / subagents_spawned and treat role/intent as supporting context instead of the trigger.
Also applies to: 63-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@talos/runtime.py` around lines 51 - 52, The drift guard in the orchestration
check is firing too early because `intent` and `main_thread_role` are being used
as the primary trigger. Update the logic around the drift check to require a
real subagent handoff by keying off `subagents_requested` and
`subagents_spawned`, and use `intent`/`main_thread_role` only as supporting
context. Make sure the same gating is applied consistently in the related drift
handling paths in `talos.runtime` so a single-thread run cannot report that
subagents were requested unless they actually were.
Comment on lines
+156
to
+164
| ratio_threshold = int(snapshot.global_max * active_policy.friction_ratio) if snapshot.global_max > 0 else 0 | ||
| if snapshot.global_max <= 0: | ||
| return ["global_max<=0 so Talos stayed at clean level 0"] | ||
| if snapshot.wip_total >= snapshot.global_max: | ||
| reasons.append("wip_total reached or exceeded global_max") | ||
| if snapshot.at_cap_projects: | ||
| reasons.append("one or more projects are already at cap") | ||
| if snapshot.wip_total >= ratio_threshold and snapshot.wip_total < snapshot.global_max: | ||
| reasons.append("wip_total crossed the friction threshold") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep explain_level() aligned with compute_talos_level().
compute_talos_level() compares against global_max * friction_ratio as a float, but this path floors that threshold with int(...). For inputs like global_max=3, friction_ratio=0.8, wip_total=2, the level stays 0 while the reason says the friction threshold was crossed. That makes evaluate_snapshot.reasons and CLI explain disagree with the scorer.
Suggested fix
- ratio_threshold = int(snapshot.global_max * active_policy.friction_ratio) if snapshot.global_max > 0 else 0
if snapshot.global_max <= 0:
return ["global_max<=0 so Talos stayed at clean level 0"]
@@
- if snapshot.wip_total >= ratio_threshold and snapshot.wip_total < snapshot.global_max:
+ if (
+ snapshot.wip_total >= snapshot.global_max * active_policy.friction_ratio
+ and snapshot.wip_total < snapshot.global_max
+ ):
reasons.append("wip_total crossed the friction threshold")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ratio_threshold = int(snapshot.global_max * active_policy.friction_ratio) if snapshot.global_max > 0 else 0 | |
| if snapshot.global_max <= 0: | |
| return ["global_max<=0 so Talos stayed at clean level 0"] | |
| if snapshot.wip_total >= snapshot.global_max: | |
| reasons.append("wip_total reached or exceeded global_max") | |
| if snapshot.at_cap_projects: | |
| reasons.append("one or more projects are already at cap") | |
| if snapshot.wip_total >= ratio_threshold and snapshot.wip_total < snapshot.global_max: | |
| reasons.append("wip_total crossed the friction threshold") | |
| if snapshot.global_max <= 0: | |
| return ["global_max<=0 so Talos stayed at clean level 0"] | |
| if snapshot.wip_total >= snapshot.global_max: | |
| reasons.append("wip_total reached or exceeded global_max") | |
| if snapshot.at_cap_projects: | |
| reasons.append("one or more projects are already at cap") | |
| if ( | |
| snapshot.wip_total >= snapshot.global_max * active_policy.friction_ratio | |
| and snapshot.wip_total < snapshot.global_max | |
| ): | |
| reasons.append("wip_total crossed the friction threshold") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@talos/runtime.py` around lines 156 - 164, Keep explain_level() consistent
with compute_talos_level() by using the same friction threshold calculation
instead of flooring it with int(...). Update the threshold check in runtime.py
so the comparison against snapshot.wip_total uses the same float-based
global_max * active_policy.friction_ratio logic as the scorer, while preserving
the early return for snapshot.global_max <= 0. This will keep the reasons
generated by explain_level() aligned with compute_talos_level() and the CLI
explain output.
Comment on lines
+157
to
+158
| if snapshot.global_max <= 0: | ||
| return ["global_max<=0 so Talos stayed at clean level 0"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't drop guardrails in the zero-cap explain path.
The early return for global_max <= 0 bypasses the later guardrails append, so an orchestration-drift snapshot can populate evaluation.guardrails but omit the same message from evaluation.reasons. That edge case breaks the new explain/output surfacing contract.
Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@talos/runtime.py` around lines 157 - 158, The zero-cap early return in the
explain path skips the later guardrails handling, so `evaluation.reasons` can
miss a message that is present in `evaluation.guardrails`. Update the relevant
flow in `talos.runtime` (the branch that checks `snapshot.global_max <= 0`, and
the similar guardrails logic later in the same path) so the same zero-cap
message is appended to reasons before returning, or otherwise ensure the
guardrails append runs for this edge case without changing the clean-level
behavior.
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.
Summary\n- add a narrow orchestration telemetry contract to Talos snapshots/evaluations\n- detect when the main thread drifts into worker implementation after subagents were requested\n- surface the guardrail in evaluation output and CLI explain output\n\n## Testing\n- ./.venv/bin/pytest\n
Summary by CodeRabbit
New Features
Bug Fixes
Documentation