Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesQuota-governed dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant QuotaDecision
participant GoalMetadata
participant Agent
Supervisor->>QuotaDecision: evaluate discovered item
QuotaDecision->>GoalMetadata: read goal state and metadata
GoalMetadata-->>QuotaDecision: return goal context
QuotaDecision-->>Supervisor: return effective action
Supervisor->>Agent: dispatch or request human gate
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (7)
src/supervisor.rs (4)
135-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the relabel logic with
skip_item.
ask_itemandskip_item(Lines 101-133) differ only in the comment heading, the comment body, and the target label name. Both post a comment, removeready_id, then conditionally add one label fromlabel_id_by_name.Extract one helper that takes the comment body and the target label name. This keeps the two paths from drifting when the labeling rules change.
🤖 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 `@src/supervisor.rs` around lines 135 - 162, Extract the shared comment-and-relabel sequence from ask_item and skip_item into a helper that accepts the comment body and target label name. Have both functions delegate to this helper while preserving their distinct comment headings, bodies, and label constants, including removal of ready_id and conditional target-label addition.
539-571: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the persisted
consecutive_self_repairsvalue.Neither test checks the goal metadata after the tick.
under_cap_self_repairs_and_dispatchesdiscards_goal_idat Line 543, andat_cap_forces_ask_instead_of_dispatchingdiscards it at Line 555.
consecutive_self_repairsis the state thatSELF_REPAIR_CAPdepends on, anddecide_for_supervisoris the only writer. Both tests assert the dispatch outcome but not the counter, so a wrong counter update passes.Add these assertions:
- After the under-cap tick, the counter is 1.
- After the at-cap tick, the counter is still at the cap.
Also run
run_discovery_ticka second time inat_cap_forces_ask_instead_of_dispatching. A single tick does not prove the gate holds. This is the same concern raised onsrc/quota/decide.rsLines 242-251.🤖 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 `@src/supervisor.rs` around lines 539 - 571, The tests must verify persisted repair-counter state and repeated gating. In under_cap_self_repairs_and_dispatches, retain goal_id and assert consecutive_self_repairs is 1 after the tick; in at_cap_forces_ask_instead_of_dispatching, retain goal_id, assert the counter remains SELF_REPAIR_CAP, then run run_discovery_tick a second time and confirm it still does not dispatch or enqueue another job while preserving the human-gate behavior.
326-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated seed helpers.
seed_ready_item_under_gated_goal(Lines 326-411) andseed_ready_item_under_active_goal_with_repairs(Lines 434-537) repeat the same label loop, state lookup, goal-item creation, child-item creation, and ready-label attach. Only the lifecycle value, the repairs value, the vent seeding, and the return type differ. Most of that block also repeatsseed_ready_item(Lines 239-290).Extract one helper that takes the lifecycle string and the repair count. Each new goal scenario otherwise copies about 80 lines.
One behavior difference to keep in mind: Line 335 and Line 446 use
let _ =onlabel::create, while Line 243 uses.unwrap(). Pick one and apply it consistently in the shared helper.🤖 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 `@src/supervisor.rs` around lines 326 - 348, Collapse seed_ready_item_under_gated_goal, seed_ready_item_under_active_goal_with_repairs, and the overlapping seed_ready_item logic into one shared helper that accepts the lifecycle value and repairs count, while preserving each scenario’s distinct vent seeding and return type through appropriate wrappers or parameters. Reuse the shared label creation, state lookup, goal-item creation, child-item creation, and ready-label attachment flow, and apply one consistent label::create error-handling strategy throughout.
87-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccount for waited items in
DiscoveryTickResult.The
Waitarm increments neitherdispatchednorskipped. The dashboard then reports no activity when all ready items are waiting.Add a
waitedfield and include it in the dashboard summary.🤖 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 `@src/supervisor.rs` around lines 87 - 91, Add a waited count to DiscoveryTickResult and increment it in the EffectiveAction::Wait branch, while preserving the existing label behavior. Update the dashboard summary to include the waited count so ticks containing only waiting items report activity.src/quota/decide.rs (3)
220-228: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReturn the goal from
decideinstead of looking it up twice.Line 221 runs
decide, which resolves the goal ancestor internally at Line 90. Line 225 then opens the database again and repeatsfind_goal_ancestorfor the same item.This costs a second parent-chain walk per item per tick. It also opens a read gap: the metadata written back at Lines 236 and 249 is based on a different read than the one
decideevaluated, so a concurrent writer can make the persisted state inconsistent with the decision.Have
decidereturn the resolved(goal_item, meta)alongside theDecision, then reuse it here.🤖 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 `@src/quota/decide.rs` around lines 220 - 228, Update decide and its caller so decide returns the resolved (goal_item, meta) together with the Decision; in the surrounding flow, destructure that result and reuse the returned goal metadata instead of calling mcp.with_backend_db with find_goal_ancestor again. Preserve fail-closed behavior when the database lookup fails and ensure the metadata writes use the same goal resolution evaluated by decide.
486-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the comment and add coverage for
decide_for_supervisor.Line 486 states the TTL is zero seconds. The code passes
1as the TTL and backdates the timestamp by 10000 seconds. Update the comment to describe the backdated heartbeat.Separately, no test in this module calls
decide_for_supervisor. That function holds all the persistence side effects: the counter increment, the counter reset, and theGatetransition. Add tests for it, including a two-tick case that proves the self-repair cap still holds after a gate.🤖 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 `@src/quota/decide.rs` around lines 486 - 494, Update the comment above the `agentflare_backend::claim::acquire` call to describe the heartbeat timestamp being backdated by 10,000 seconds, not a zero-second TTL. Add module tests invoking `decide_for_supervisor` that cover counter increment, counter reset, and `Gate` transitions, including a two-tick scenario verifying the self-repair cap remains enforced after a gate.
144-172: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the full project scan for each ready item.
run_discovery_tickcallsdecideonce per ready item. Tier 3 then loads every item in the project and performs onestate::getfor each candidate sibling. This creates O(ready items × project items) work per tick. Add a child query that joins thestartedstate, or cache sibling and state data for the tick.🤖 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 `@src/quota/decide.rs` around lines 144 - 172, Optimize the Tier 3 sibling check in decide/run_discovery_tick to avoid scanning all project items and calling state::get for every ready item. Add or reuse a child-item query that filters by the goal parent and joins the started state, or cache the sibling/state data once per tick; preserve the existing stale-claim detection and Decision::wait behavior.
🤖 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 `@src/quota/decide.rs`:
- Line 97: Update the vent lookup in decide to fail closed when
agentflare_backend::vent::list returns an error, matching the existing ancestor
lookup handling near line 92. Do not continue to lower-tier evaluation or allow
Decision::run to proceed when the query fails; preserve the current tier-1
behavior for successful results.
- Around line 242-251: In the EffectiveActionInternal::Ask branch, stop
resetting consecutive_self_repairs when raising the Gate; reset it only in the
lifecycle-clear path alongside LifecycleEvent::Clear. Replace ignored save
results in the self-repair metadata update and Ask-path persistence with
appropriate propagation or logging, and add a supervisor regression test
covering two ticks to ensure the cap continues forcing Ask.
- Line 106: Update the high-severity check in the linked-vent logic to compare
normalize_severity(Some(&v.severity)) against "high" instead of comparing the
stored severity directly, preserving the existing any-based gate and
normalization behavior such as treating "critical" as "medium".
- Around line 177-184: Update the claim-owner comparison in the decision logic
around has_active_claim_by_other so it compares identities at the intended agent
scope rather than passing item.assignee_agent directly, which omits the instance
suffix used by claims. Ensure same-agent claims such as claude-code:1 are
recognized correctly, and add a regression test covering that scenario.
In `@src/quota/goal.rs`:
- Around line 52-57: Update the metadata handling in the goal update function
around serde_json::from_str and value["goal"]: preserve parsed object metadata,
but return an error when parsing fails or when the parsed value is not a JSON
object, rather than defaulting to {} or indexing directly. Keep the existing
serialization error propagation and ensure the caller fails closed consistently
with parse_goal_metadata.
---
Nitpick comments:
In `@src/quota/decide.rs`:
- Around line 220-228: Update decide and its caller so decide returns the
resolved (goal_item, meta) together with the Decision; in the surrounding flow,
destructure that result and reuse the returned goal metadata instead of calling
mcp.with_backend_db with find_goal_ancestor again. Preserve fail-closed behavior
when the database lookup fails and ensure the metadata writes use the same goal
resolution evaluated by decide.
- Around line 486-494: Update the comment above the
`agentflare_backend::claim::acquire` call to describe the heartbeat timestamp
being backdated by 10,000 seconds, not a zero-second TTL. Add module tests
invoking `decide_for_supervisor` that cover counter increment, counter reset,
and `Gate` transitions, including a two-tick scenario verifying the self-repair
cap remains enforced after a gate.
- Around line 144-172: Optimize the Tier 3 sibling check in
decide/run_discovery_tick to avoid scanning all project items and calling
state::get for every ready item. Add or reuse a child-item query that filters by
the goal parent and joins the started state, or cache the sibling/state data
once per tick; preserve the existing stale-claim detection and Decision::wait
behavior.
In `@src/supervisor.rs`:
- Around line 135-162: Extract the shared comment-and-relabel sequence from
ask_item and skip_item into a helper that accepts the comment body and target
label name. Have both functions delegate to this helper while preserving their
distinct comment headings, bodies, and label constants, including removal of
ready_id and conditional target-label addition.
- Around line 539-571: The tests must verify persisted repair-counter state and
repeated gating. In under_cap_self_repairs_and_dispatches, retain goal_id and
assert consecutive_self_repairs is 1 after the tick; in
at_cap_forces_ask_instead_of_dispatching, retain goal_id, assert the counter
remains SELF_REPAIR_CAP, then run run_discovery_tick a second time and confirm
it still does not dispatch or enqueue another job while preserving the
human-gate behavior.
- Around line 326-348: Collapse seed_ready_item_under_gated_goal,
seed_ready_item_under_active_goal_with_repairs, and the overlapping
seed_ready_item logic into one shared helper that accepts the lifecycle value
and repairs count, while preserving each scenario’s distinct vent seeding and
return type through appropriate wrappers or parameters. Reuse the shared label
creation, state lookup, goal-item creation, child-item creation, and ready-label
attachment flow, and apply one consistent label::create error-handling strategy
throughout.
- Around line 87-91: Add a waited count to DiscoveryTickResult and increment it
in the EffectiveAction::Wait branch, while preserving the existing label
behavior. Update the dashboard summary to include the waited count so ticks
containing only waiting items report activity.
🪄 Autofix
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 Plus
Run ID: d19b0e02-c7cc-4183-b867-4103a15044c3
📒 Files selected for processing (6)
src/main.rssrc/quota/decide.rssrc/quota/goal.rssrc/quota/lifecycle.rssrc/quota/mod.rssrc/supervisor.rs
|
|
||
| if let Some((goal_item, goal_meta)) = &goal { | ||
| // Tier 1: health-gate. | ||
| if let Ok(vents) = agentflare_backend::vent::list(conn, &goal_item.project_id, true) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when the vent query fails.
Line 97 discards the error from vent::list. If the read fails, tier 1 is skipped entirely and decide continues to lower tiers. The item can then reach Decision::run while a high-severity friction report is linked to the goal.
Line 92 already fails closed for the ancestor lookup. Apply the same posture here.
🛡️ Proposed fix
- if let Ok(vents) = agentflare_backend::vent::list(conn, &goal_item.project_id, true) {
+ let vents = match agentflare_backend::vent::list(conn, &goal_item.project_id, true) {
+ Ok(vents) => vents,
+ Err(e) => {
+ return Decision::fail_closed(format!("cannot read friction reports: {e}"));
+ }
+ };
+ {
let linked: Vec<_> = vents📝 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.
| if let Ok(vents) = agentflare_backend::vent::list(conn, &goal_item.project_id, true) { | |
| let vents = match agentflare_backend::vent::list(conn, &goal_item.project_id, true) { | |
| Ok(vents) => vents, | |
| Err(e) => { | |
| return Decision::fail_closed(format!("cannot read friction reports: {e}")); | |
| } | |
| }; | |
| { | |
| let linked: Vec<_> = vents |
🤖 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 `@src/quota/decide.rs` at line 97, Update the vent lookup in decide to fail
closed when agentflare_backend::vent::list returns an error, matching the
existing ancestor lookup handling near line 92. Do not continue to lower-tier
evaluation or allow Decision::run to proceed when the query fails; preserve the
current tier-1 behavior for successful results.
| }) | ||
| .collect(); | ||
| if !linked.is_empty() { | ||
| let has_high = linked.iter().any(|v| v.severity == "high"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect normalize_severity and every severity comparison/write.
set -euo pipefail
ast-grep run --pattern 'fn normalize_severity($$$) { $$$ }' --lang rust src || true
rg -n -C5 'fn normalize_severity' src crates
echo "== severity comparisons and writes =="
rg -n -C3 '\bseverity\b' --type=rust src crates -g '!**/tests/**'Repository: getappz/agentflare
Length of output: 28892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== quota decision context =="
sed -n '1,145p' src/quota/decide.rs
echo "== all vent persistence callers =="
rg -n -C4 'append_routed|append\(|upsert\(' src crates -g '*.rs' -g '!**/tests/**'
echo "== vent schema and migrations =="
rg -n -C5 'CREATE TABLE.*vents|severity.*TEXT|INSERT INTO vents|UPDATE vents SET severity' src crates -g '*.rs' -g '*.sql'
echo "== direct construction of VentLine and Vent values =="
rg -n -C3 'VentLine\s*\{|Vent\s*\{' src crates -g '*.rs' -g '!**/tests/**'Repository: getappz/agentflare
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend vent upsert =="
sed -n '1,175p' crates/agentflare-backend/src/vent.rs
echo "== consolidation input and severity flow =="
sed -n '1,150p' src/vent/consolidate.rs
echo "== database schema locations =="
rg -l 'CREATE TABLE[^(]*vents|CREATE TABLE IF NOT EXISTS vents|vents \(' src crates -g '*.rs' -g '*.sql' |
while read -r f; do
echo "--- $f"
rg -n -C6 'CREATE TABLE.*vents|severity.*TEXT|vents \(' "$f"
done
echo "== non-test production calls to backend vent::upsert =="
python3 - <<'PY'
from pathlib import Path
for p in list(Path("src").rglob("*.rs")) + list(Path("crates").rglob("*.rs")):
text = p.read_text()
if "agentflare_backend::vent::upsert(" not in text:
continue
lines = text.splitlines()
for i, line in enumerate(lines):
if "agentflare_backend::vent::upsert(" in line and not any("cfg(test)" in x for x in lines[max(0, i-20):i]):
print(f"{p}:{i+1}")
PYRepository: getappz/agentflare
Length of output: 14350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== backend vent upsert =="
sed -n '1,175p' crates/agentflare-backend/src/vent.rs
echo "== consolidation input and severity flow =="
sed -n '1,150p' src/vent/consolidate.rs
echo "== database schema locations =="
rg -l 'CREATE TABLE[^(]*vents|CREATE TABLE IF NOT EXISTS vents|vents \(' src crates -g '*.rs' -g '*.sql' |
while read -r f; do
echo "--- $f"
rg -n -C6 'CREATE TABLE.*vents|severity.*TEXT|vents \(' "$f"
done
echo "== non-test production calls to backend vent::upsert =="
python3 - <<'PY'
from pathlib import Path
for p in list(Path("src").rglob("*.rs")) + list(Path("crates").rglob("*.rs")):
text = p.read_text()
if "agentflare_backend::vent::upsert(" not in text:
continue
lines = text.splitlines()
for i, line in enumerate(lines):
if "agentflare_backend::vent::upsert(" in line:
print(f"{p}:{i+1}")
PYRepository: getappz/agentflare
Length of output: 14350
Normalize stored severity before the high-severity comparison.
vents.severity is unconstrained TEXT, and agentflare_backend::vent::upsert stores caller-provided values. consolidate_core forwards VentLine.severity, so "High" can bypass this gate. Compare normalize_severity(Some(&v.severity)) with "high". "critical" currently normalizes to "medium", not "high".
🤖 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 `@src/quota/decide.rs` at line 106, Update the high-severity check in the
linked-vent logic to compare normalize_severity(Some(&v.severity)) against
"high" instead of comparing the stored severity directly, preserving the
existing any-based gate and normalization behavior such as treating "critical"
as "medium".
| let this_owner = item.assignee_agent.as_deref().unwrap_or(""); | ||
| if agentflare_backend::claim::has_active_claim_by_other( | ||
| conn, &item.id, this_owner, now, ttl_secs, | ||
| ) | ||
| .unwrap_or(false) | ||
| { | ||
| return Decision::wait("another agent already holds a live claim on this item"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine how claim owner strings are constructed and compared.
set -euo pipefail
fd -t f 'claim.rs' crates/agentflare-backend/src | while IFS= read -r f; do
echo "== $f"
cat -n "$f"
done
echo "== claim owner construction in the binary crate =="
rg -n -C4 'claim::acquire|current_owner|has_active_claim_by_other' src
rg -n -C4 'fn owner|owner_id|claim_owner' src cratesRepository: getappz/agentflare
Length of output: 30263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== claims::owner_id and identity helpers =="
sed -n '240,285p' src/claims.rs
echo "== quota decision implementation and focused tests =="
sed -n '130,190p' src/quota/decide.rs
sed -n '450,525p' src/quota/decide.rs
echo "== assignee_agent construction and decision callers =="
rg -n -C4 'assignee_agent|decide\(' src cratesRepository: getappz/agentflare
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
claims = Path("src/claims.rs").read_text()
decide = Path("src/quota/decide.rs").read_text()
owner = claims[claims.index("pub fn owner_id()"):claims.index("pub fn agent_of")]
print("== owner_id implementation ==")
print(owner)
print("== tier 4 call ==")
m = re.search(r"// Tier 4:.*?has_active_claim_by_other\(\s*conn,\s*&item\.id,\s*this_owner,\s*now,\s*ttl_secs,", decide, re.S)
print(m.group(0) if m else "tier 4 call not found")
print("== identity-shape probe ==")
# Mirrors the exact comparison in has_active_claim_by_other.
for assignee, claim_owner in [
("claude-code", "claude-code:1"),
("claude-code:1", "claude-code:1"),
("claude-code", "codex:1"),
]:
result = claim_owner != assignee
print(f"assignee_agent={assignee!r}, claim_owner={claim_owner!r}, "
f"has_active_claim_by_other={result}")
PYRepository: getappz/agentflare
Length of output: 1542
Do not pass item.assignee_agent as the claim owner. has_active_claim_by_other performs an exact owner comparison, but claims use <agent>:<instance> and assignee_agent stores only <agent>. Thus claude-code:1 is treated as different from claude-code. Compare identities at the intended scope and add a same-agent regression test.
🤖 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 `@src/quota/decide.rs` around lines 177 - 184, Update the claim-owner
comparison in the decision logic around has_active_claim_by_other so it compares
identities at the intended agent scope rather than passing item.assignee_agent
directly, which omits the instance suffix used by claims. Ensure same-agent
claims such as claude-code:1 are recognized correctly, and add a regression test
covering that scenario.
| EffectiveActionInternal::Ask => { | ||
| if let Some((goal_item, mut meta)) = goal { | ||
| meta.consecutive_self_repairs = 0; | ||
| if let Ok(next) = meta.lifecycle.apply(super::lifecycle::LifecycleEvent::Gate) { | ||
| meta.lifecycle = next; | ||
| } | ||
| let _ = mcp.with_backend_db(|conn| { | ||
| super::goal::save_goal_metadata(conn, &goal_item.id, &meta) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The Ask branch resets the self-repair counter and defeats SELF_REPAIR_CAP.
Line 244 sets consecutive_self_repairs = 0 for every Ask. One Ask reason is the cap itself: decide returns Ask at Lines 113-119 because consecutive_self_repairs >= SELF_REPAIR_CAP while friction is still linked. Resetting the counter at that moment erases the state that produced the gate.
The next tick then loops:
- The friction report is still linked, so tier 1 runs at Line 105.
consecutive_self_repairsis now 0, so the cap check at Line 113 is false.decidereturnsSelfRepairat Line 121, and the counter climbs to 3 again.
The Gated lifecycle does not stop this. Tier 1 at Line 96 runs before the tier 2 lifecycle check at Line 128, so the gate is never reached while friction stays linked. The cap therefore never holds, and the goal self-repairs without bound.
Reset the counter only when the gate is cleared, not when it is raised.
A second path produces the same outcome. Lines 235 and 248 discard the save result with let _ =. If the save fails, the increment at Line 234 is lost, and consecutive_self_repairs never reaches the cap. Propagate or log that failure.
The supervisor test at_cap_forces_ask_instead_of_dispatching in src/supervisor.rs Lines 551-571 runs a single tick, so it does not cover the second-tick behavior. Add a two-tick test.
🐛 Proposed fix
EffectiveActionInternal::Ask => {
if let Some((goal_item, mut meta)) = goal {
- meta.consecutive_self_repairs = 0;
if let Ok(next) = meta.lifecycle.apply(super::lifecycle::LifecycleEvent::Gate) {
meta.lifecycle = next;
}
- let _ = mcp.with_backend_db(|conn| {
- super::goal::save_goal_metadata(conn, &goal_item.id, &meta)
- });
+ if let Err(e) = mcp.with_backend_db(|conn| {
+ super::goal::save_goal_metadata(conn, &goal_item.id, &meta)
+ }) {
+ tracing::warn!(goal = %goal_item.id, error = %e, "could not persist goal gate state");
+ }
}Clear consecutive_self_repairs where the gate is cleared, alongside the LifecycleEvent::Clear transition.
🤖 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 `@src/quota/decide.rs` around lines 242 - 251, In the
EffectiveActionInternal::Ask branch, stop resetting consecutive_self_repairs
when raising the Gate; reset it only in the lifecycle-clear path alongside
LifecycleEvent::Clear. Replace ignored save results in the self-repair metadata
update and Ask-path persistence with appropriate propagation or logging, and add
a supervisor regression test covering two ticks to ensure the cap continues
forcing Ask.
| let mut value: serde_json::Value = | ||
| serde_json::from_str(&item.metadata).unwrap_or_else(|_| serde_json::json!({})); | ||
| value["goal"] = | ||
| serde_json::to_value(goal).map_err(|e| format!("goal metadata does not serialize: {e}"))?; | ||
| let updated = | ||
| serde_json::to_string(&value).map_err(|e| format!("metadata does not serialize: {e}"))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle non-object metadata instead of assuming an object.
Two problems share one cause at Line 52: the code assumes item.metadata is a JSON object without proving it.
- Line 53 replaces unparsable metadata with
{}. Every sibling key is then lost. This contradicts the doc comment at Lines 42-44, which promises to preserve other keys. - Line 54 indexes
value["goal"]. If the metadata parses as a valid non-object value, for example[]or3,serde_jsonpanics on index assignment.serde_jsononly auto-promotesValue::Nullto an object. The panic propagates into the supervisor discovery tick throughdecide_for_supervisor.
Return an error for both cases so the caller fails closed, which matches parse_goal_metadata.
🛡️ Proposed fix
- let mut value: serde_json::Value =
- serde_json::from_str(&item.metadata).unwrap_or_else(|_| serde_json::json!({}));
- value["goal"] =
- serde_json::to_value(goal).map_err(|e| format!("goal metadata does not serialize: {e}"))?;
+ let mut value: serde_json::Value = if item.metadata.trim().is_empty() {
+ serde_json::json!({})
+ } else {
+ serde_json::from_str(&item.metadata)
+ .map_err(|e| format!("existing metadata on {goal_item_id} is not valid json: {e}"))?
+ };
+ let object = value.as_object_mut().ok_or_else(|| {
+ format!("existing metadata on {goal_item_id} is not a json object")
+ })?;
+ object.insert(
+ "goal".to_string(),
+ serde_json::to_value(goal).map_err(|e| format!("goal metadata does not serialize: {e}"))?,
+ );📝 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.
| let mut value: serde_json::Value = | |
| serde_json::from_str(&item.metadata).unwrap_or_else(|_| serde_json::json!({})); | |
| value["goal"] = | |
| serde_json::to_value(goal).map_err(|e| format!("goal metadata does not serialize: {e}"))?; | |
| let updated = | |
| serde_json::to_string(&value).map_err(|e| format!("metadata does not serialize: {e}"))?; | |
| let mut value: serde_json::Value = if item.metadata.trim().is_empty() { | |
| serde_json::json!({}) | |
| } else { | |
| serde_json::from_str(&item.metadata) | |
| .map_err(|e| format!("existing metadata on {goal_item_id} is not valid json: {e}"))? | |
| }; | |
| let object = value.as_object_mut().ok_or_else(|| { | |
| format!("existing metadata on {goal_item_id} is not a json object") | |
| })?; | |
| object.insert( | |
| "goal".to_string(), | |
| serde_json::to_value(goal).map_err(|e| format!("goal metadata does not serialize: {e}"))?, | |
| ); | |
| let updated = | |
| serde_json::to_string(&value).map_err(|e| format!("metadata does not serialize: {e}"))?; |
🤖 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 `@src/quota/goal.rs` around lines 52 - 57, Update the metadata handling in the
goal update function around serde_json::from_str and value["goal"]: preserve
parsed object metadata, but return an error when parsing fails or when the
parsed value is not a JSON object, rather than defaulting to {} or indexing
directly. Keep the existing serialization error propagation and ensure the
caller fails closed consistently with parse_goal_metadata.
cargo clippy -D warnings failed the build: LifecycleEvent::{Pause,Resume,Clear,Complete}
have no production caller yet (only Gate is used, by decide_for_supervisor's Ask branch) --
they're only exercised from lifecycle.rs's own #[cfg(test)] tests, which the lib target's
dead_code analysis doesn't count as usage.
Also runs rustfmt on lifecycle.rs and supervisor.rs, which had drifted from the repo's
formatting since the last style commit (a collapsed single-line assert!, import order).
Local rustfmt is 1.9.0-stable; CI's dtolnay/rust-toolchain@stable resolves newer and disagrees with it on both of these -- confirmed against the actual `fmt` job's failure diff on PR #392, not by re-guessing locally: lifecycle.rs's assert!() wants to stay expanded, and supervisor.rs wants AgentflareMcp imported before types.
getappz
changed the title
1 participant
Auto-opened on
item donefor 5TyjlcAYFutRV8JX7GxO7.Summary by CodeRabbit
New Features
Bug Fixes