feat: orchestrator + acceptance criteria#16
Closed
devzeebo wants to merge 27 commits into
Closed
Conversation
- Capture stdout from dispatched commands to extract completion stats - Parse JSON telemetry (duration, tokens, cost, turns) from agent output - Format stats into human-readable note with [orchestrator] attribution - Append note before fulfilling rune on successful execution - No note appended on agent failure - All bf-10af tests now passing Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This reverts commit 61978a6.
Contributor
|
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:
📝 WalkthroughSummary by CodeRabbitRelease Notes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
141-146:⚠️ Potential issue | 🟡 MinorUpdate
helptext for the newtest-py/lint-pytargets.The help output still says
test/lintonly cover "Go + UI" and omits the new Python targets entirely.🔧 Suggested fix
- `@echo` " test Run all tests (Go + UI)" + `@echo` " test Run all tests (Go + UI + Python)" `@echo` " test-go Run Go tests (all modules or MODULES=...)" `@echo` " test-ui Run UI tests (vitest)" - `@echo` " lint Run all linters (Go + UI)" + `@echo` " test-py Run Python tests (pytest under claude-orchestrator/)" + `@echo` " lint Run all linters (Go + UI + Python)" `@echo` " lint-go Run golangci-lint (all modules or MODULES=...)" `@echo` " lint-ui Run oxlint" + `@echo` " lint-py Run ruff check --fix and ruff format (claude-orchestrator/)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 141 - 146, Update the Makefile help text lines for the test and lint targets to mention the new Python targets: modify the echo strings for "test", "test-go", "test-ui", "lint", "lint-go", and "lint-ui" (and add mentions of "test-py" and "lint-py") so the help output shows that test/lint cover "Go + UI + Python" and lists the new per-language targets "test-py" and "lint-py" alongside the existing ones.
♻️ Duplicate comments (1)
claude-orchestrator/test_agent_integration.py (1)
1-480:⚠️ Potential issue | 🔴 CriticalCritical: same broken-import issue as
test_completion_note_e2e.py— tests cannot collect.
_run_agentexists inagent.py, butformat_completion_note,append_completion_note_to_api, andpost_to_apido not (telemetry commit reverted). Every test class in this file imports at least one of these, so pytest collection will fail before a single test executes. This will break themake test-pytarget added in this PR.See the root-cause comment on
test_completion_note_e2e.pyfor remediation options (delete both test files, or re-land the telemetry helpers). Flagging here so both files aren't missed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/test_agent_integration.py` around lines 1 - 480, Tests fail to collect because agent.py no longer exports format_completion_note, append_completion_note_to_api, and post_to_api (only _run_agent remains); re-introduce minimal implementations in agent.py with the same names so tests can import them: implement format_completion_note(stats) to return a readable string containing duration_ms, token counts, cache_read_tokens/cache_creation_tokens, total_cost_usd (formatted to 4 decimals) and num_turns; implement append_completion_note_to_api(rune_id, text, api_base) to call post_to_api(endpoint="/add-note", payload={"rune_id": rune_id, "text": text}, base=api_base) and swallow exceptions; implement post_to_api(endpoint, payload=None, base=None) as a simple wrapper that performs an HTTP POST or a test-friendly stub that raises on real failures (matching tests which patch it); ensure function names/signatures match tests so imports succeed without modifying _run_agent.
🧹 Nitpick comments (9)
cli/show.go (1)
81-91: Minor: innerid/descshadow outer variables.The
id, _ := acMap["id"].(string)anddesc, _ := acMap["description"].(string)inside the loop shadow the rune-levelid/descdeclared earlier in the closure. Works correctly today but is easy to misread during future edits; consider renaming (e.g.,acID,acDesc).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/show.go` around lines 81 - 91, The loop over acceptance criteria shadows outer variables by using id and desc; rename the inner variables to avoid shadowing (e.g., acID and acDesc) in the block where you extract from acMap (the lines reading acMap["id"] and acMap["description"]) and update the fmt.Fprintf call inside the loop to use acID and acDesc; keep the same type assertions and behavior but use distinct names instead of id/desc to prevent shadowing.cli/update.go (1)
78-123: Consider consolidating the three AC flag handlers.The three blocks are near-identical (decode JSON, inject
rune_id, POST to a path); extracting a small helper would reduce repetition and keep error-message formatting consistent. Not blocking.Also note that
update-runefollowed by N AC posts is not atomic — a later AC call failing will leave the rune partially updated. Acceptable for a CLI, but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/update.go` around lines 78 - 123, Consolidate the three near-identical AC flag handlers in cli/update.go by extracting a small helper (e.g., postACItems or handleACFlag) that accepts the flag name and target endpoint ("/add-ac", "/update-ac", "/remove-ac"), iterates values returned by cmd.Flags().GetStringArray(flag), parses JSON for the add/update flags or builds a map for remove, injects "rune_id" = id, and calls clientFn().DoPost(endpoint, body); use that helper from the existing places so parsing/error formatting is consistent and duplicate code is removed while preserving existing error returns.cli/show_ac_test.go (1)
192-199: Reinventsstrings.Index.
indexOfInStringis juststrings.Indexwith extra allocations and no rune-awareness. Prefer the stdlib.♻️ Proposed fix
-func indexOfInString(s, substr string) int { - for i := range s { - if i+len(substr) <= len(s) && s[i:i+len(substr)] == substr { - return i - } - } - return -1 -} +// use strings.Index directly at call sitesAnd at the call sites:
- idx01 := indexOfInString(output, "AC-01") - idx02 := indexOfInString(output, "AC-02") - idx03 := indexOfInString(output, "AC-03") + idx01 := strings.Index(output, "AC-01") + idx02 := strings.Index(output, "AC-02") + idx03 := strings.Index(output, "AC-03")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/show_ac_test.go` around lines 192 - 199, The helper indexOfInString duplicates stdlib behavior and is not rune-aware; remove the indexOfInString function and replace its call sites to use strings.Index from the standard library (add/import "strings" where needed). Update references to indexOfInString to strings.Index(s, substr) and delete the custom function definition to avoid extra allocations and correctness issues with multi-byte runes.domain/projectors/rune_detail.go (1)
341-360:handleACUpdatedbumpsUpdatedAtand writes even if the AC ID is missing.If the event's
data.IDisn't present inAcceptanceCriteria(e.g. out-of-order replay, or a bug upstream), the loop is a no-op but the projector still updatesUpdatedAtand re-Puts the detail. In practice the domain handler guards this, but making the projector a no-op when the entry isn't found (same shape ashandleDependencyRemoved’s natural filter semantics) would be more defensive against replay drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/projectors/rune_detail.go` around lines 341 - 360, The projector currently always sets detail.UpdatedAt and calls store.Put in handleACUpdated even when no AcceptanceCriteria entry matches data.ID; change handleACUpdated to search AcceptanceCriteria for a matching ac.ID, and if none is found do nothing (return nil) instead of updating UpdatedAt or calling store.Put. Only modify detail.AcceptanceCriteria, update detail.UpdatedAt and call store.Put when a match was found (use the existing loop over detail.AcceptanceCriteria and a boolean/early-return to detect whether to persist).domain/handlers.go (2)
742-760: Duplicated existence-scan logic betweenHandleUpdateACItemandHandleRemoveACItem.The two blocks (Lines 742–760 and 789–807) are identical. Consider extracting a small helper like
acExistsInEvents(events []core.Event, acID string) boolto keep the two handlers in sync as AC semantics evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/handlers.go` around lines 742 - 760, There is duplicated logic scanning events to determine AC existence in HandleUpdateACItem and HandleRemoveACItem; extract a helper function (e.g., acExistsInEvents(events []core.Event, acID string) bool) that encapsulates the loop and JSON unmarshalling for EventRuneACAdded/EventRuneACRemoved and use that helper from both handlers (replace the duplicated blocks with a call to acExistsInEvents), keeping the same semantics (returns true if last relevant event for that ID is Added, false if Removed or absent).
694-711: Simplify AC-ID parsing.The manual length + slice check for
"AC-"is a minor readability nit —strings.HasPrefixis idiomatic, andfmt.Sscanfalready safely reports no match if the prefix or format doesn't hold, so the guard is mostly defensive.♻️ Proposed tweak
- // Parse AC-NN format - if len(data.ID) > 3 && data.ID[:3] == "AC-" { - var num int - _, _ = fmt.Sscanf(data.ID, "AC-%d", &num) - if num > maxID { - maxID = num - } - } + // Parse AC-NN format + if strings.HasPrefix(data.ID, "AC-") { + var num int + if _, err := fmt.Sscanf(data.ID, "AC-%d", &num); err == nil && num > maxID { + maxID = num + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/handlers.go` around lines 694 - 711, Replace the manual length + slice check used to detect the "AC-" prefix with the idiomatic strings.HasPrefix when scanning AC IDs: inside the loop that iterates events (checking evt.EventType against EventRuneACAdded and EventRuneACRemoved), use strings.HasPrefix(data.ID, "AC-") before calling fmt.Sscanf on data.ID to parse the numeric suffix and update maxID; you can drop the len(data.ID) > 3 guard because Sscanf will fail safely if the format doesn't match, ensuring nextID (constructed as fmt.Sprintf("AC-%02d", maxID+1)) remains correct.cli/update_ac_test.go (1)
318-328:request_to_path_has_fieldonly inspects the first match.If a test later needs to verify fields on the second request to the same path (e.g. multiple
--ac-add), this helper silently stops at the first match and can hide mismatches on later requests. Current tests don't exercise that, but worth noting if coverage grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/update_ac_test.go` around lines 318 - 328, The helper request_to_path_has_field stops at the first request matching path and ignores later requests; change it to scan all tc.requests with req.path == path and assert that at least one matching request has req.body[key] == expected (or alternatively add an occurrence/index parameter to check a specific matching request). Update the logic in request_to_path_has_field to collect matching reqs from tc.requests, require non-empty matches, then either assert the nth match equals expected when an index parameter is provided or assert that any match satisfies req.body[key] == expected using the existing require/assert helpers (referencing tc.requests, req.path, req.body, key, expected, and the request_to_path_has_field function name).claude-orchestrator/agents/loader.py (1)
176-182:toolslist entries are assumed to be strings.If a user writes
tools: [1, 2]or a list with aNone,t.strip()will raiseAttributeErrorand the whole agent file gets discarded (caught by the blind-except above, so the failure is silent). Coercing withstr()makes this robust.🔧 Suggested fix
- if isinstance(tools_raw, list): - tools = [t.strip() for t in tools_raw if t] or None + if isinstance(tools_raw, list): + tools = [str(t).strip() for t in tools_raw if t] or None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agents/loader.py` around lines 176 - 182, The tools parsing assumes list entries are strings and calls t.strip(), which raises AttributeError for non-string entries (e.g., tools: [1, None])—update the logic in loader.py (the block handling tools_raw and producing tools) to coerce each entry to str before stripping and to skip empty results; specifically change the list branch that builds tools from tools_raw to use something like [str(t).strip() for t in tools_raw if t is not None and str(t).strip()] so non-strings are safely handled and blank entries are filtered, leaving tools as None when no valid entries exist.Makefile (1)
69-71: PassARGSthrough to pytest for parity withtest-go.
test-goforwards$(ARGS)(as advertised inhelp), buttest-pyignores it, somake test ARGS="-k foo -x"won't filter Python tests.🔧 Suggested fix
test-py: `@echo` "» uv test" - cd claude-orchestrator && uv run python -m pytest + cd claude-orchestrator && uv run python -m pytest $(ARGS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 69 - 71, The test-py Makefile target ignores the ARGS variable so `make test ARGS="..."` doesn't filter Python tests; update the test-py recipe to pass $(ARGS) through to the pytest invocation (similar to test-go) by appending $(ARGS) to the command run in the test-py target (referencing the test-py target and the ARGS variable and the pytest invocation "uv run python -m pytest") so that provided ARGS are forwarded to pytest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.bifrost.yaml:
- Around line 3-6: Replace the developer-specific absolute dispatcher path in
.bifrost.yaml with the repository-relative orchestrator script or remove the
block; specifically update the dispatcher key (currently pointing to
/home/devzeebo/.config/bifrost/orchestrator) to reference the in-tree script
claude-orchestrator/dispatcher.py (or omit the orchestrate/dispatcher entry so
local untracked overrides can be used), ensuring the value is a repo-relative
path so CI and other contributors can resolve it.
- Line 6: The .bifrost.yaml contains an unsupported key "logging: verbose" that
is ignored because Viper's Unmarshal is used; either remove the key from the
YAML or add a Logging field to the OrchestrateConfig struct in
cli/orchestrate.go (with the proper `mapstructure:"logging"` tag and an
appropriate type, e.g., string or map[string]interface{}) so the setting is
recognized at runtime; update any related usage sites that read
OrchestrateConfig to handle the new field if you choose to implement it.
In `@claude-orchestrator/agent.py`:
- Around line 338-381: The while-True loop in _run_rune_stop_hooks can loop
forever on repeated exit-code 1 follow-ups; add a bounded retry counter (e.g.
MAX_RUNE_STOP_RETRIES constant) and a local retry_count variable in
_run_rune_stop_hooks, increment retry_count each time you handle
result.returncode == 1 (after sending the follow_up and draining messages via
client and _drain_messages), and if retry_count exceeds MAX_RUNE_STOP_RETRIES
log a clear error via logger/_log_hook and return False, last_ns to abort; reset
retry_count to 0 when you get a full successful pass (all hooks return 0) or
when a hook returns 2, and keep using existing symbols _run_hook_command,
rune_stop_hooks, _drain_messages, client, last_ns, and _log_hook to locate and
implement the change.
In `@claude-orchestrator/agents/loader.py`:
- Around line 82-90: The broad except in the agent-loading try/except (around
the call to _parse_agent_file and the loaded[name] assignment) is intentional;
silence Ruff BLE001 by adding a per-line noqa comment on the except clause (e.g.
except Exception as exc: # noqa: BLE001) and include a short inline comment
explaining the intent (to continue loading other agent files on parse errors) so
the linter is satisfied while preserving the catch-all behavior.
In `@claude-orchestrator/dispatcher.py`:
- Around line 74-79: The printed hook lists show HookCommand(...) because the
code joins str(p) for each element; change the projection to use the
NamedTuple's .command field instead (e.g., join p.command or str(p.command)) in
the rune_start and rune_stop print lines so the output shows the actual command
path; update the two locations referencing hooks.rune_start and hooks.rune_stop
(and the generator expressions producing str(p)) to reference p.command instead.
In `@claude-orchestrator/pyproject.toml`:
- Around line 9-21: Remove "ruff" from the runtime dependencies list and add it
to the dev optional-dependencies; update the test tooling minimums by raising
pytest, pytest-asyncio, and pytest-mock to current minima (e.g. set
"pytest>=8.1", "pytest-asyncio>=0.23", "pytest-mock>=3.11") under
[project.optional-dependencies] -> dev, leaving runtime dependencies containing
only "claude-agent-sdk", "pyyaml", and "watchdog".
In `@claude-orchestrator/test_agent_integration.py`:
- Around line 102-104: The assertions like "assert mock_post.called or True" are
always true and therefore ineffective; update each occurrence (the one shown and
the similar occurrences around lines 356–357 and 475–476) to either remove the
"or True" so the test actually asserts mock_post.called once the helper is
implemented, or explicitly mark the test as skipped/xfail until implementation
exists (use pytest.skip or pytest.mark.xfail with a clear message) so the test
outcome is visible; ensure you modify the test function(s) containing
mock_post.called to use the chosen approach consistently.
In `@claude-orchestrator/test_completion_note_e2e.py`:
- Line 40: The assertions using loose checks on the string note (e.g., assert
"1" in note and "0" in note, assert "1" in note, and checks with "cache" in
note.lower()) are too weak; replace them with deterministic assertions that
validate the exact formatting contract produced by the generator. Update the
tests that reference the note variable to assert specific substrings or patterns
(for example: assert "input=1" in note, assert re.search(r"\$\d+\.\d{4}", note)
for cost formatting, or assert re.search(r"\d{1,3}(,\d{3})*", note) for
comma-separated numbers) or construct an expected_note from the same producing
function and compare equality; target the assertion changes around the existing
checks that contain the literal snippets "1" and "0" and the branch using
"cache" so the numeric and formatting components are explicitly verified.
- Around line 1-458: Tests import and patch symbols that don't exist
(format_completion_note, append_completion_note_to_api, post_to_api), causing
collection to fail; either remove these test files or restore minimal
implementations in agent.py. To fix by restoring, add small functions: define
format_completion_note(stats) that returns a stable human-readable string
containing "orchestrator", cost with "$", duration and token counts
(JSON-serializable); define post_to_api(url, payload, headers=None) that
performs a simple requests.post wrapper (or raises ConnectionError for tests);
and define append_completion_note_to_api(rune_id, note, api_base) that builds
JSON payload, calls post_to_api, and handles/propagates exceptions so tests can
patch post_to_api; ensure names exactly match format_completion_note,
append_completion_note_to_api, and post_to_api so imports/patches succeed.
In `@claude-orchestrator/test_completion_note.py`:
- Around line 1-361: The tests fail because the imported functions
format_completion_note, append_completion_note_to_api, and post_to_api no longer
exist in agent.py; either remove these orphan telemetry tests (delete
test_completion_note.py and related telemetry tests like
test_completion_note_e2e.py) or restore the three functions and their append
call-site in agent.py (implement format_completion_note(stats),
append_completion_note_to_api(rune_id, text, base_url) and post_to_api(endpoint,
payload) with the minimal API contract the tests expect); also ensure the `@patch`
targets in the tests match actual attributes on the agent module to avoid
AttributeError during collection.
In `@cli/create.go`:
- Around line 81-101: The code currently silently skips `--ac-add` processing if
json.Unmarshal(respBody, &createdRune) fails or if the `"id"` field isn't a
string; change this so failures are surfaced: after calling
json.Unmarshal(respBody, &createdRune) check the error and return a descriptive
error if unmarshal fails, and if the `"id"` key is missing or not a string
return an error indicating the created rune ID could not be extracted
(especially when cmd.Flags().Changed("ac-add") is true); keep using the same
symbols (json.Unmarshal, respBody, createdRune, runeID,
cmd.Flags().Changed("ac-add"), clientFn().DoPost("/add-ac", ...)) but ensure you
return errors instead of silently skipping AC posts so callers know AC creation
must be retried or inspected.
In `@domain/projectors/rune_ac_counter.go`:
- Around line 43-51: The current code swallows the error from store.Get when
loading ACCounter which can turn transient backend errors into a silent reset of
counter; modify the block around store.Get(ctx, event.RealmID,
"rune_ac_counter", data.RuneID, &counter) to inspect the returned error: if it's
a NotFound (or equivalent sentinel from the store) treat that as a fresh counter
and continue, but for any other error return/propagate that error instead of
proceeding to increment and call store.Put; keep ACCounter, counter, store.Get
and store.Put, event.RealmID, "rune_ac_counter" and data.RuneID as the
referenced symbols so the fix only distinguishes NotFound vs other errors and
avoids overwriting on transient failures.
In `@server/ac_integration_test.go`:
- Around line 17-122: The E2E failures are caused because the AC HTTP endpoints
aren't registered; add three HTTP handler methods in server/handlers.go (e.g.,
ServeAddACItem, ServeUpdateACItem, ServeRemoveACItem) that follow the pattern of
existing member-auth endpoints (like the POST /api/create-rune handler): decode
the JSON body into the same request DTOs used by domain handlers, call
domain.HandleAddACItem / HandleUpdateACItem / HandleRemoveACItem, handle domain
errors by mapping *core.NotFoundError → 404 and state-gate errors → 422, and
write appropriate JSON/no-content responses; then register these handlers in
RegisterRoutes() for POST /api/add-ac, POST /api/update-ac, and POST
/api/remove-ac so the auth middleware is exercised.
---
Outside diff comments:
In `@Makefile`:
- Around line 141-146: Update the Makefile help text lines for the test and lint
targets to mention the new Python targets: modify the echo strings for "test",
"test-go", "test-ui", "lint", "lint-go", and "lint-ui" (and add mentions of
"test-py" and "lint-py") so the help output shows that test/lint cover "Go + UI
+ Python" and lists the new per-language targets "test-py" and "lint-py"
alongside the existing ones.
---
Duplicate comments:
In `@claude-orchestrator/test_agent_integration.py`:
- Around line 1-480: Tests fail to collect because agent.py no longer exports
format_completion_note, append_completion_note_to_api, and post_to_api (only
_run_agent remains); re-introduce minimal implementations in agent.py with the
same names so tests can import them: implement format_completion_note(stats) to
return a readable string containing duration_ms, token counts,
cache_read_tokens/cache_creation_tokens, total_cost_usd (formatted to 4
decimals) and num_turns; implement append_completion_note_to_api(rune_id, text,
api_base) to call post_to_api(endpoint="/add-note", payload={"rune_id": rune_id,
"text": text}, base=api_base) and swallow exceptions; implement
post_to_api(endpoint, payload=None, base=None) as a simple wrapper that performs
an HTTP POST or a test-friendly stub that raises on real failures (matching
tests which patch it); ensure function names/signatures match tests so imports
succeed without modifying _run_agent.
---
Nitpick comments:
In `@claude-orchestrator/agents/loader.py`:
- Around line 176-182: The tools parsing assumes list entries are strings and
calls t.strip(), which raises AttributeError for non-string entries (e.g.,
tools: [1, None])—update the logic in loader.py (the block handling tools_raw
and producing tools) to coerce each entry to str before stripping and to skip
empty results; specifically change the list branch that builds tools from
tools_raw to use something like [str(t).strip() for t in tools_raw if t is not
None and str(t).strip()] so non-strings are safely handled and blank entries are
filtered, leaving tools as None when no valid entries exist.
In `@cli/show_ac_test.go`:
- Around line 192-199: The helper indexOfInString duplicates stdlib behavior and
is not rune-aware; remove the indexOfInString function and replace its call
sites to use strings.Index from the standard library (add/import "strings" where
needed). Update references to indexOfInString to strings.Index(s, substr) and
delete the custom function definition to avoid extra allocations and correctness
issues with multi-byte runes.
In `@cli/show.go`:
- Around line 81-91: The loop over acceptance criteria shadows outer variables
by using id and desc; rename the inner variables to avoid shadowing (e.g., acID
and acDesc) in the block where you extract from acMap (the lines reading
acMap["id"] and acMap["description"]) and update the fmt.Fprintf call inside the
loop to use acID and acDesc; keep the same type assertions and behavior but use
distinct names instead of id/desc to prevent shadowing.
In `@cli/update_ac_test.go`:
- Around line 318-328: The helper request_to_path_has_field stops at the first
request matching path and ignores later requests; change it to scan all
tc.requests with req.path == path and assert that at least one matching request
has req.body[key] == expected (or alternatively add an occurrence/index
parameter to check a specific matching request). Update the logic in
request_to_path_has_field to collect matching reqs from tc.requests, require
non-empty matches, then either assert the nth match equals expected when an
index parameter is provided or assert that any match satisfies req.body[key] ==
expected using the existing require/assert helpers (referencing tc.requests,
req.path, req.body, key, expected, and the request_to_path_has_field function
name).
In `@cli/update.go`:
- Around line 78-123: Consolidate the three near-identical AC flag handlers in
cli/update.go by extracting a small helper (e.g., postACItems or handleACFlag)
that accepts the flag name and target endpoint ("/add-ac", "/update-ac",
"/remove-ac"), iterates values returned by cmd.Flags().GetStringArray(flag),
parses JSON for the add/update flags or builds a map for remove, injects
"rune_id" = id, and calls clientFn().DoPost(endpoint, body); use that helper
from the existing places so parsing/error formatting is consistent and duplicate
code is removed while preserving existing error returns.
In `@domain/handlers.go`:
- Around line 742-760: There is duplicated logic scanning events to determine AC
existence in HandleUpdateACItem and HandleRemoveACItem; extract a helper
function (e.g., acExistsInEvents(events []core.Event, acID string) bool) that
encapsulates the loop and JSON unmarshalling for
EventRuneACAdded/EventRuneACRemoved and use that helper from both handlers
(replace the duplicated blocks with a call to acExistsInEvents), keeping the
same semantics (returns true if last relevant event for that ID is Added, false
if Removed or absent).
- Around line 694-711: Replace the manual length + slice check used to detect
the "AC-" prefix with the idiomatic strings.HasPrefix when scanning AC IDs:
inside the loop that iterates events (checking evt.EventType against
EventRuneACAdded and EventRuneACRemoved), use strings.HasPrefix(data.ID, "AC-")
before calling fmt.Sscanf on data.ID to parse the numeric suffix and update
maxID; you can drop the len(data.ID) > 3 guard because Sscanf will fail safely
if the format doesn't match, ensuring nextID (constructed as
fmt.Sprintf("AC-%02d", maxID+1)) remains correct.
In `@domain/projectors/rune_detail.go`:
- Around line 341-360: The projector currently always sets detail.UpdatedAt and
calls store.Put in handleACUpdated even when no AcceptanceCriteria entry matches
data.ID; change handleACUpdated to search AcceptanceCriteria for a matching
ac.ID, and if none is found do nothing (return nil) instead of updating
UpdatedAt or calling store.Put. Only modify detail.AcceptanceCriteria, update
detail.UpdatedAt and call store.Put when a match was found (use the existing
loop over detail.AcceptanceCriteria and a boolean/early-return to detect whether
to persist).
In `@Makefile`:
- Around line 69-71: The test-py Makefile target ignores the ARGS variable so
`make test ARGS="..."` doesn't filter Python tests; update the test-py recipe to
pass $(ARGS) through to the pytest invocation (similar to test-go) by appending
$(ARGS) to the command run in the test-py target (referencing the test-py target
and the ARGS variable and the pytest invocation "uv run python -m pytest") so
that provided ARGS are forwarded to pytest.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3fe058c-4fd9-47ff-8f5d-53f63940ebb7
⛔ Files ignored due to path filters (1)
claude-orchestrator/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.bifrost.yaml.gitignoreMakefileclaude-orchestrator/CLAUDE.mdclaude-orchestrator/agent.pyclaude-orchestrator/agents/__init__.pyclaude-orchestrator/agents/loader.pyclaude-orchestrator/dispatcher.pyclaude-orchestrator/pyproject.tomlclaude-orchestrator/test_agent_integration.pyclaude-orchestrator/test_completion_note.pyclaude-orchestrator/test_completion_note_e2e.pycli/create.gocli/create_ac_test.gocli/show.gocli/show_ac_test.gocli/update.gocli/update_ac_test.godomain/ac_handlers_test.godomain/commands.godomain/events.godomain/handlers.godomain/handlers_test.godomain/projectors/rune_ac_counter.godomain/projectors/rune_detail.godomain/projectors/rune_detail_ac_test.goserver/ac_integration_test.goserver/main.go
Comment on lines
+3
to
+6
| orchestrate: | ||
| dispatcher: /home/devzeebo/.config/bifrost/orchestrator | ||
| concurrency: 1 | ||
| logging: verbose |
Contributor
There was a problem hiding this comment.
Avoid committing a developer-specific absolute path.
dispatcher: /home/devzeebo/.config/bifrost/orchestrator is a hard-coded personal path that won't resolve for any other contributor, CI agent, or docker build; bf orchestrate will fail immediately with dispatcher script not found. Since the orchestrator lives in-repo at claude-orchestrator/dispatcher.py, point .bifrost.yaml at the in-tree script (or drop this block from the committed config and keep the absolute path in a local/untracked override).
🔧 Suggested fix
orchestrate:
- dispatcher: /home/devzeebo/.config/bifrost/orchestrator
+ dispatcher: claude-orchestrator/dispatcher.py
concurrency: 1
logging: verbose📝 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
| orchestrate: | |
| dispatcher: /home/devzeebo/.config/bifrost/orchestrator | |
| concurrency: 1 | |
| logging: verbose | |
| orchestrate: | |
| dispatcher: claude-orchestrator/dispatcher.py | |
| concurrency: 1 | |
| logging: verbose |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.bifrost.yaml around lines 3 - 6, Replace the developer-specific absolute
dispatcher path in .bifrost.yaml with the repository-relative orchestrator
script or remove the block; specifically update the dispatcher key (currently
pointing to /home/devzeebo/.config/bifrost/orchestrator) to reference the
in-tree script claude-orchestrator/dispatcher.py (or omit the
orchestrate/dispatcher entry so local untracked overrides can be used), ensuring
the value is a repo-relative path so CI and other contributors can resolve it.
| orchestrate: | ||
| dispatcher: /home/devzeebo/.config/bifrost/orchestrator | ||
| concurrency: 1 | ||
| logging: verbose |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\b(Logging|logging|Verbose|verbose)\b' --type=go -g '!**/vendor/**'
rg -nP -C3 'OrchestrateConfig|orchestrate\.' --type=go -g '!**/vendor/**'Repository: devzeebo/bifrost
Length of output: 2306
🏁 Script executed:
# Read the complete OrchestrateConfig struct definition
cat -n cli/orchestrate.go | sed -n '17,30p'Repository: devzeebo/bifrost
Length of output: 680
🏁 Script executed:
# Find YAML unmarshalling code
rg -n 'Unmarshal|mapstructure|yaml' --type=go -A3 -B3 cli/config.goRepository: devzeebo/bifrost
Length of output: 1731
🏁 Script executed:
# Check if Logging is referenced anywhere in the codebase
rg -n 'Logging|logging' --type=go -C2Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Check Viper configuration for field handling
rg -n 'v\.' cli/config.go | head -20Repository: devzeebo/bifrost
Length of output: 445
🏁 Script executed:
# Look for any error handling related to unknown fields
rg -n 'unknown|unmapped|extra.*field|field.*extra' --type=go -iRepository: devzeebo/bifrost
Length of output: 9764
🌐 Web query:
Viper library default behavior unknown fields YAML unmarshalling
💡 Result:
The Viper library's default behavior for YAML unmarshalling regarding unknown fields is to ignore them silently. Viper reads YAML (or other formats) into its internal map[string]any representation, then uses the mapstructure library's Unmarshal (via viper.Unmarshal) to decode that map into a Go struct. By default, mapstructure ignores keys in the source map that do not match any struct fields, without error. To error on unknown fields instead, use viper.UnmarshalExact, which configures mapstructure with ErrorUnused=true, causing it to fail if there are unmapped keys. This applies specifically after YAML parsing; the YAML decoder itself (used by Viper for reading config files) is lenient and does not strict-unmarshal to structs directly—YAML goes to the map first.
Citations:
- 1: https://deepwiki.com/spf13/viper/4.2-unmarshaling-to-structs
- 2: UnmarshalExact() returns misleading error message spf13/viper#1724
- 3: https://github.com/spf13/viper/
- 4: https://pkg.go.dev/mod/github.com/go-viper/mapstructure/v2
Remove or implement support for logging: verbose.
The Logging field is not present in OrchestrateConfig (cli/orchestrate.go). Because the code uses Viper's standard .Unmarshal() method (not .UnmarshalExact()), this unsupported key is silently ignored at runtime rather than raising a config error. Either remove logging: verbose from .bifrost.yaml if it's unintended, or add a Logging field to OrchestrateConfig with appropriate mapstructure tag if it should be honored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.bifrost.yaml at line 6, The .bifrost.yaml contains an unsupported key
"logging: verbose" that is ignored because Viper's Unmarshal is used; either
remove the key from the YAML or add a Logging field to the OrchestrateConfig
struct in cli/orchestrate.go (with the proper `mapstructure:"logging"` tag and
an appropriate type, e.g., string or map[string]interface{}) so the setting is
recognized at runtime; update any related usage sites that read
OrchestrateConfig to handle the new field if you choose to implement it.
Comment on lines
+338
to
+381
| while True: | ||
| restarted = False | ||
| for hook in rune_stop_hooks: | ||
| try: | ||
| result = _run_hook_command(hook.command, rune_json, cwd) | ||
| except Exception as exc: | ||
| logger.warning("hook:RuneStop command=%s failed: %s", hook.command, exc) | ||
| continue | ||
|
|
||
| hook_output = result.stdout.strip() or result.stderr.strip() | ||
|
|
||
| if result.returncode == 0: | ||
| _log_hook("RuneStop", hook.command, 0) | ||
| continue | ||
|
|
||
| if result.returncode == 1: | ||
| _log_hook("RuneStop", hook.command, 1, hook_output) | ||
| follow_up = ( | ||
| "A post-completion hook reported an issue and provided " | ||
| f"additional context. Please review and address it:\n\n{hook_output}" | ||
| ) | ||
| await client.query(follow_up) | ||
| cont_result, last_ns = await _drain_messages( | ||
| client, rune_id, agent_name, verbose, start_ns=last_ns | ||
| ) | ||
| if not cont_result: | ||
| logger.error( | ||
| "Agent %r produced no ResultMessage after hook follow-up", | ||
| agent_name, | ||
| ) | ||
| return False, last_ns | ||
| # Restart all hooks from scratch to verify the fix | ||
| restarted = True | ||
| break | ||
|
|
||
| elif result.returncode == 2: | ||
| _log_hook("RuneStop", hook.command, 2, hook_output) | ||
| return False, last_ns | ||
|
|
||
| else: | ||
| _log_hook("RuneStop", hook.command, result.returncode, hook_output) | ||
|
|
||
| if not restarted: | ||
| return True, last_ns |
Contributor
There was a problem hiding this comment.
Potential infinite loop: _run_rune_stop_hooks has no max-retry guard on exit-code-1 follow-ups.
The while True loop restarts the full hook suite from scratch after every exit-1 follow-up, and only exits when either (a) every hook returns 0 in a single pass or (b) a hook returns 2. If a RuneStop hook (e.g. a flaky lint/test command) keeps returning 1 and the agent keeps producing a ResultMessage that doesn't actually fix the issue, this will loop indefinitely, burning tokens and wall-clock until the SDK session dies or the process is killed externally.
Suggested guard:
♻️ Bound the retry count
+MAX_RUNE_STOP_RETRIES = 5
+
async def _run_rune_stop_hooks(
rune_stop_hooks: list,
rune_json: str,
cwd: str,
client,
rune_id: str,
agent_name: str,
verbose: bool,
last_ns: int,
) -> tuple[bool, int]:
- while True:
+ retries = 0
+ while True:
restarted = False
for hook in rune_stop_hooks:
...
if result.returncode == 1:
...
+ retries += 1
+ if retries > MAX_RUNE_STOP_RETRIES:
+ logger.error(
+ "RuneStop hook %s still failing after %d retries; giving up",
+ hook.command, MAX_RUNE_STOP_RETRIES,
+ )
+ return False, last_ns
restarted = True
break🧰 Tools
🪛 Ruff (0.15.10)
[warning] 343-343: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 338 - 381, The while-True loop in
_run_rune_stop_hooks can loop forever on repeated exit-code 1 follow-ups; add a
bounded retry counter (e.g. MAX_RUNE_STOP_RETRIES constant) and a local
retry_count variable in _run_rune_stop_hooks, increment retry_count each time
you handle result.returncode == 1 (after sending the follow_up and draining
messages via client and _drain_messages), and if retry_count exceeds
MAX_RUNE_STOP_RETRIES log a clear error via logger/_log_hook and return False,
last_ns to abort; reset retry_count to 0 when you get a full successful pass
(all hooks return 0) or when a hook returns 2, and keep using existing symbols
_run_hook_command, rune_stop_hooks, _drain_messages, client, last_ns, and
_log_hook to locate and implement the change.
Comment on lines
+82
to
+90
| try: | ||
| name, entry = _parse_agent_file(path) | ||
| loaded[name] = entry | ||
| logger.debug("Loaded agent %r from %s", name, path) | ||
| except Exception as exc: | ||
| logger.warning("Failed to load agent from %s: %s", path, exc) | ||
| with self._lock: | ||
| self._agents = loaded | ||
| logger.info("Loaded %d agent(s): %s", len(loaded), list(loaded.keys())) |
Contributor
There was a problem hiding this comment.
Broad except Exception is intentional here, but silence the ruff warning.
Ruff (BLE001) flags Line 86. Keeping a file-level catch-all makes sense so one malformed agent file doesn't kill loading of the rest — just document intent with a noqa so the lint stays clean.
🔧 Suggested fix
- except Exception as exc:
+ except Exception as exc: # noqa: BLE001 — isolate per-file parse failures
logger.warning("Failed to load agent from %s: %s", path, exc)🧰 Tools
🪛 Ruff (0.15.10)
[warning] 86-86: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agents/loader.py` around lines 82 - 90, The broad except
in the agent-loading try/except (around the call to _parse_agent_file and the
loaded[name] assignment) is intentional; silence Ruff BLE001 by adding a
per-line noqa comment on the except clause (e.g. except Exception as exc: #
noqa: BLE001) and include a short inline comment explaining the intent (to
continue loading other agent files on parse errors) so the linter is satisfied
while preserving the catch-all behavior.
Comment on lines
+74
to
+79
| if hooks.rune_start: | ||
| print( | ||
| f" rune_start_hooks: {', '.join(str(p) for p in hooks.rune_start)}" | ||
| ) | ||
| if hooks.rune_stop: | ||
| print(f" rune_stop_hooks: {', '.join(str(p) for p in hooks.rune_stop)}") |
Contributor
There was a problem hiding this comment.
--list-agents prints HookCommand(command='…') instead of the command itself.
hooks.rune_start/rune_stop are list[HookCommand] (a NamedTuple). str(p) on a NamedTuple returns its repr, so the output looks like HookCommand(command='./pre.sh') rather than the command path users expect. Project the .command field directly.
🔧 Suggested fix
if hooks.rune_start:
print(
- f" rune_start_hooks: {', '.join(str(p) for p in hooks.rune_start)}"
+ f" rune_start_hooks: {', '.join(h.command for h in hooks.rune_start)}"
)
if hooks.rune_stop:
- print(f" rune_stop_hooks: {', '.join(str(p) for p in hooks.rune_stop)}")
+ print(f" rune_stop_hooks: {', '.join(h.command for h in hooks.rune_stop)}")📝 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
| if hooks.rune_start: | |
| print( | |
| f" rune_start_hooks: {', '.join(str(p) for p in hooks.rune_start)}" | |
| ) | |
| if hooks.rune_stop: | |
| print(f" rune_stop_hooks: {', '.join(str(p) for p in hooks.rune_stop)}") | |
| if hooks.rune_start: | |
| print( | |
| f" rune_start_hooks: {', '.join(h.command for h in hooks.rune_start)}" | |
| ) | |
| if hooks.rune_stop: | |
| print(f" rune_stop_hooks: {', '.join(h.command for h in hooks.rune_stop)}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/dispatcher.py` around lines 74 - 79, The printed hook
lists show HookCommand(...) because the code joins str(p) for each element;
change the projection to use the NamedTuple's .command field instead (e.g., join
p.command or str(p.command)) in the rune_start and rune_stop print lines so the
output shows the actual command path; update the two locations referencing
hooks.rune_start and hooks.rune_stop (and the generator expressions producing
str(p)) to reference p.command instead.
| assert isinstance(note, str) | ||
| assert len(note) > 0 | ||
| # Should contain million or properly formatted numbers | ||
| assert "1" in note and "0" in note # Parts of "1000000" or formatted |
Contributor
There was a problem hiding this comment.
Assertions are too loose to catch real regressions.
A few checks reduce to tautologies or near-tautologies and won't catch formatting regressions even once the feature exists:
- Line 40:
assert "1" in note and "0" in note— trivially true for almost any note containing a duration or cost. - Line 78:
... or "cache" in note.lower()— the right-hand branch is true whenever the word "cache" appears anywhere, masking bugs in the numeric portion. - Line 137:
assert "1" in note— same tautology.
Consider asserting against the actual formatting contract (e.g., specific substrings like "input=1", "$0.0000", "1,500") once the producing function is defined.
Also applies to: 78-78, 137-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/test_completion_note_e2e.py` at line 40, The assertions
using loose checks on the string note (e.g., assert "1" in note and "0" in note,
assert "1" in note, and checks with "cache" in note.lower()) are too weak;
replace them with deterministic assertions that validate the exact formatting
contract produced by the generator. Update the tests that reference the note
variable to assert specific substrings or patterns (for example: assert
"input=1" in note, assert re.search(r"\$\d+\.\d{4}", note) for cost formatting,
or assert re.search(r"\d{1,3}(,\d{3})*", note) for comma-separated numbers) or
construct an expected_note from the same producing function and compare
equality; target the assertion changes around the existing checks that contain
the literal snippets "1" and "0" and the branch using "cache" so the numeric and
formatting components are explicitly verified.
Comment on lines
+81
to
+101
| // Extract created rune ID for AC operations | ||
| var createdRune map[string]any | ||
| if json.Unmarshal(respBody, &createdRune) == nil { | ||
| if runeID, ok := createdRune["id"].(string); ok { | ||
| // Handle --ac-add flags | ||
| if cmd.Flags().Changed("ac-add") { | ||
| acAddJSONs, _ := cmd.Flags().GetStringArray("ac-add") | ||
| for _, acJSON := range acAddJSONs { | ||
| var acBody map[string]any | ||
| if err := json.Unmarshal([]byte(acJSON), &acBody); err != nil { | ||
| return fmt.Errorf("invalid JSON for --ac-add: %s", acJSON) | ||
| } | ||
| acBody["rune_id"] = runeID | ||
| _, err := clientFn().DoPost("/add-ac", acBody) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent skip of --ac-add when response parsing fails.
If json.Unmarshal(respBody, ...) errors, or if id isn't a string, the AC operations are silently skipped and the command still exits successfully — the user will see "Created rune …" but none of their acceptance criteria were posted. Since --ac-add being present implies the user expects ACs to be created, this should surface as an error rather than be ignored.
🛠️ Proposed fix
- // Extract created rune ID for AC operations
- var createdRune map[string]any
- if json.Unmarshal(respBody, &createdRune) == nil {
- if runeID, ok := createdRune["id"].(string); ok {
- // Handle --ac-add flags
- if cmd.Flags().Changed("ac-add") {
- acAddJSONs, _ := cmd.Flags().GetStringArray("ac-add")
- for _, acJSON := range acAddJSONs {
- var acBody map[string]any
- if err := json.Unmarshal([]byte(acJSON), &acBody); err != nil {
- return fmt.Errorf("invalid JSON for --ac-add: %s", acJSON)
- }
- acBody["rune_id"] = runeID
- _, err := clientFn().DoPost("/add-ac", acBody)
- if err != nil {
- return err
- }
- }
- }
- }
- }
+ if cmd.Flags().Changed("ac-add") {
+ var createdRune map[string]any
+ if err := json.Unmarshal(respBody, &createdRune); err != nil {
+ return fmt.Errorf("parse create-rune response: %w", err)
+ }
+ runeID, ok := createdRune["id"].(string)
+ if !ok || runeID == "" {
+ return fmt.Errorf("create-rune response missing id; cannot apply --ac-add")
+ }
+ acAddJSONs, _ := cmd.Flags().GetStringArray("ac-add")
+ for _, acJSON := range acAddJSONs {
+ var acBody map[string]any
+ if err := json.Unmarshal([]byte(acJSON), &acBody); err != nil {
+ return fmt.Errorf("invalid JSON for --ac-add: %s", acJSON)
+ }
+ acBody["rune_id"] = runeID
+ if _, err := clientFn().DoPost("/add-ac", acBody); err != nil {
+ return err
+ }
+ }
+ }Separately, note that create + N × add-ac are not atomic — a mid-sequence failure leaves a partially-populated rune. Worth documenting, and eventually considering a batched endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/create.go` around lines 81 - 101, The code currently silently skips
`--ac-add` processing if json.Unmarshal(respBody, &createdRune) fails or if the
`"id"` field isn't a string; change this so failures are surfaced: after calling
json.Unmarshal(respBody, &createdRune) check the error and return a descriptive
error if unmarshal fails, and if the `"id"` key is missing or not a string
return an error indicating the created rune ID could not be extracted
(especially when cmd.Flags().Changed("ac-add") is true); keep using the same
symbols (json.Unmarshal, respBody, createdRune, runeID,
cmd.Flags().Changed("ac-add"), clientFn().DoPost("/add-ac", ...)) but ensure you
return errors instead of silently skipping AC posts so callers know AC creation
must be retried or inspected.
Comment on lines
+43
to
+51
| // Get or create counter for this rune | ||
| counter := ACCounter{Count: 0} | ||
| _ = store.Get(ctx, event.RealmID, "rune_ac_counter", data.RuneID, &counter) | ||
|
|
||
| // Note: ID parsing is done in handlers.go, here we just increment the counter | ||
| // The counter represents the highest AC number ever issued for this rune | ||
| counter.Count++ | ||
|
|
||
| return store.Put(ctx, event.RealmID, "rune_ac_counter", data.RuneID, counter) |
Contributor
There was a problem hiding this comment.
Swallowed store.Get error can silently reset the counter.
The discarded error from store.Get conflates a legitimate not-found (first AC) with transient backend errors (serialization issues, connection failures, decode errors). On any non-NotFound failure, counter stays at its zero value and you overwrite the stored counter with 1, regressing "the highest AC number ever issued" and breaking idempotent replay guarantees.
🛡️ Proposed fix
counter := ACCounter{Count: 0}
- _ = store.Get(ctx, event.RealmID, "rune_ac_counter", data.RuneID, &counter)
+ if err := store.Get(ctx, event.RealmID, "rune_ac_counter", data.RuneID, &counter); err != nil {
+ var nfe *core.NotFoundError
+ if !errors.As(err, &nfe) {
+ return err
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@domain/projectors/rune_ac_counter.go` around lines 43 - 51, The current code
swallows the error from store.Get when loading ACCounter which can turn
transient backend errors into a silent reset of counter; modify the block around
store.Get(ctx, event.RealmID, "rune_ac_counter", data.RuneID, &counter) to
inspect the returned error: if it's a NotFound (or equivalent sentinel from the
store) treat that as a fresh counter and continue, but for any other error
return/propagate that error instead of proceeding to increment and call
store.Put; keep ACCounter, counter, store.Get and store.Put, event.RealmID,
"rune_ac_counter" and data.RuneID as the referenced symbols so the fix only
distinguishes NotFound vs other errors and avoids overwriting on transient
failures.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
claude-orchestrator/agent.py (1)
44-44: Dead declaration:_test_clientContextVar is never read or written.Nothing in this file references
_test_client— test injection is done viamonkeypatch/_client_factoryinstead. Remove it (and the unusedcontextvarsimport on line 31) to avoid confusion about how tests are supposed to inject clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` at line 44, Remove the dead ContextVar declaration and unused import: delete the ContextVar import from the top (the `contextvars` import) and remove the `_test_client: ContextVar = ContextVar("_test_client", default=None)` declaration; tests use `_client_factory`/monkeypatch for injection, so no ContextVar is needed—remove both the import and the `_test_client` symbol to avoid confusion.claude-orchestrator/conftest.py (2)
53-60: Nit: wrong noqa code and single-shot generator.Two small issues here:
# noqa: F501silences the "%-format string" rule, which has nothing to do with an unreachableyield. If you want to suppress anything it would beB901/a custom reason; as written the noqa is misleading.MagicMock(return_value=empty_messages_generator())instantiates the generator once at fixture setup. Any test that callsreceive_messages()a second time will get the already-exhausted generator. Usingside_effect=lambda: empty_messages_generator()makes each call yield a fresh (empty) generator.♻️ Proposed tweak
- # Create an async generator that yields nothing - async def empty_messages_generator(): - return - yield # noqa: F501 Make it a generator - - # Set receive_messages to return the async generator (not call the function) - mock_client.receive_messages = MagicMock( - return_value=empty_messages_generator() - ) + # Async generator that yields nothing; create a fresh one per call + async def empty_messages_generator(): + if False: + yield # pragma: no cover — makes this a generator + + mock_client.receive_messages = MagicMock( + side_effect=lambda: empty_messages_generator() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/conftest.py` around lines 53 - 60, The empty_messages_generator is currently declared with a misleading noqa and is instantiated once causing exhausted generators on repeated calls; update the generator by removing or replacing the wrong "# noqa: F501" (either delete it or use the correct suppression token if needed) on the unreachable yield line in empty_messages_generator, and change mock_client.receive_messages to use MagicMock(side_effect=lambda: empty_messages_generator()) instead of MagicMock(return_value=empty_messages_generator()) so each call returns a fresh generator.
26-61: Autouse fixture hard-coded to a single test name is fragile.
auto_patch_sdk_client_for_failing_testruns for every test in the package but only performs work when the node name containstest_agent_does_not_append_note_on_failure. This tightly couples shared configuration to one test's name — renaming/moving the test silently disables the patch (and any analogous test won't benefit from it). The docstring even acknowledges the test "doesn't patch ClaudeSDKClient", which is the real defect to fix.Prefer making the target test explicitly request a regular fixture (e.g., reuse
patch_claude_sdk_client_for_agent_testand extend it to accept a message list, or add a dedicatedpatched_sdk_client_no_messagesfixture) and drop the autouse special-case entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/conftest.py` around lines 26 - 61, The autouse fixture auto_patch_sdk_client_for_failing_test should not hard-code a test name; remove autouse=True and the special-case branch and instead provide an explicit fixture (e.g., patched_sdk_client_no_messages or extend patch_claude_sdk_client_for_agent_test to accept a messages parameter) that constructs the same mock context-manager, async empty generator, and monkeypatch of agent.ClaudeSDKClient; update the failing test to request that fixture so the patching is explicit and not tied to the test name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude-orchestrator/agent.py`:
- Around line 506-516: post_to_api currently catches Exception and only logs,
which contradicts its docstring and prevents callers like
append_completion_note_to_api from handling network/API errors; change
post_to_api to stop swallowing exceptions (remove the broad try/except or
re-raise after logging) so requests/network errors propagate to callers, and
update its docstring to still state it raises request exceptions; ensure
append_completion_note_to_api and any other callers (e.g., the completion note
caller around append_completion_note_to_api) rely on that behavior so their
existing try/except can handle/log failures as intended.
- Around line 223-240: _build_prompt currently accesses rune['id'] and
rune['title'] directly which can raise KeyError for malformed input; update it
to validate required keys at the top (check that rune.get("id") and
rune.get("title") are present) and either return a safe placeholder prompt or
raise a controlled exception with a clear message; alternatively (if you prefer
minimal change) use rune.get("id", "unknown") and rune.get("title", "untitled")
instead of direct indexing so behavior matches the defensive access used in
main(). Ensure the change references the _build_prompt function and the rune
dict keys "id" and "title".
---
Nitpick comments:
In `@claude-orchestrator/agent.py`:
- Line 44: Remove the dead ContextVar declaration and unused import: delete the
ContextVar import from the top (the `contextvars` import) and remove the
`_test_client: ContextVar = ContextVar("_test_client", default=None)`
declaration; tests use `_client_factory`/monkeypatch for injection, so no
ContextVar is needed—remove both the import and the `_test_client` symbol to
avoid confusion.
In `@claude-orchestrator/conftest.py`:
- Around line 53-60: The empty_messages_generator is currently declared with a
misleading noqa and is instantiated once causing exhausted generators on
repeated calls; update the generator by removing or replacing the wrong "# noqa:
F501" (either delete it or use the correct suppression token if needed) on the
unreachable yield line in empty_messages_generator, and change
mock_client.receive_messages to use MagicMock(side_effect=lambda:
empty_messages_generator()) instead of
MagicMock(return_value=empty_messages_generator()) so each call returns a fresh
generator.
- Around line 26-61: The autouse fixture auto_patch_sdk_client_for_failing_test
should not hard-code a test name; remove autouse=True and the special-case
branch and instead provide an explicit fixture (e.g.,
patched_sdk_client_no_messages or extend patch_claude_sdk_client_for_agent_test
to accept a messages parameter) that constructs the same mock context-manager,
async empty generator, and monkeypatch of agent.ClaudeSDKClient; update the
failing test to request that fixture so the patching is explicit and not tied to
the test name.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7039a3d-25f7-4533-8e58-9f3a808d583f
⛔ Files ignored due to path filters (1)
claude-orchestrator/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
claude-orchestrator/agent.pyclaude-orchestrator/conftest.pyclaude-orchestrator/pyproject.tomlcli/orchestrate_dispatch.goserver/handlers.goserver/handlers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- claude-orchestrator/pyproject.toml
Comment on lines
+506
to
+516
| # Append completion note only if hooks also passed | ||
| if hooks_passed: | ||
| try: | ||
| note_text = format_completion_note(stats) | ||
| api_url = os.environ.get("BIFROST_API_URL", "http://localhost:8000") | ||
| append_completion_note_to_api(rune_id, note_text, api_url) | ||
| except Exception as exc: | ||
| logger.warning("Failed to append completion note: %s", exc) | ||
|
|
||
| # Return success only if hooks passed (we already confirmed result was valid above) | ||
| return hooks_passed |
Contributor
There was a problem hiding this comment.
post_to_api silently swallows exceptions, which contradicts its docstring and makes the caller's try/except dead code.
post_to_api's docstring says it "Raises: Exceptions from requests library on network/API errors", but the implementation catches Exception and only logs a warning. As a result:
- Callers that rely on documented behavior (line 508-513 wraps
append_completion_note_to_apiintry/exceptexpecting failures to propagate) never see network/HTTP errors — theexceptbranch there is effectively unreachable for API failures. - The success path on line 516 returns
hooks_passedeven though the completion note may never have been posted. The rune will be fulfilled with no record of the note.
Pick one contract and stick with it. Given the caller already logs its own warning, the cleanest fix is to let post_to_api raise and rely on the caller to decide what's fatal:
♻️ Proposed fix
def post_to_api(url: str, payload: dict) -> None:
"""
Make an HTTP POST request to the Bifrost API.
-
- Args:
- url: Full URL endpoint (e.g., "http://localhost:8000/api/add-note")
- payload: JSON payload to send
-
- Raises:
- Exceptions from requests library on network/API errors
"""
import requests
- try:
- response = requests.post(url, json=payload, timeout=30)
- response.raise_for_status()
- except Exception as exc:
- logger.warning("Failed to POST to %s: %s", url, exc)
+ response = requests.post(url, json=payload, timeout=30)
+ response.raise_for_status()If the intent is instead to never surface API errors, then remove the redundant try/except wrapper at lines 508-513 and update the docstring to reflect "logs warnings on failure; does not raise".
Also applies to: 608-625
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 512-512: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 506 - 516, post_to_api currently
catches Exception and only logs, which contradicts its docstring and prevents
callers like append_completion_note_to_api from handling network/API errors;
change post_to_api to stop swallowing exceptions (remove the broad try/except or
re-raise after logging) so requests/network errors propagate to callers, and
update its docstring to still state it raises request exceptions; ensure
append_completion_note_to_api and any other callers (e.g., the completion note
caller around append_completion_note_to_api) rely on that behavior so their
existing try/except can handle/log failures as intended.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
claude-orchestrator/agent.py (2)
530-535:⚠️ Potential issue | 🟠 MajorKeep
post_to_api’s exception contract consistent.
post_to_api()documents that request errors are raised, but it catches everything and only logs. That makes the caller’stry/exceptineffective and can report success even when the completion note was not posted.♻️ Proposed contract fix
def post_to_api(url: str, payload: dict) -> None: @@ """ import requests - try: - response = requests.post(url, json=payload, timeout=30) - response.raise_for_status() - except Exception as exc: - logger.warning("Failed to POST to %s: %s", url, exc) + response = requests.post(url, json=payload, timeout=30) + response.raise_for_status()Also applies to: 630-647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 530 - 535, The post_to_api error handling swallows all exceptions which breaks the documented contract and makes callers like append_completion_note_to_api (used in the try/except around format_completion_note and append_completion_note_to_api) believe a failure succeeded; update post_to_api to stop catching broad Exception—only catch and wrap specific transient/network exceptions if you need to add context, then re-raise them (or raise a new, documented exception type) so callers can handle failures; adjust append_completion_note_to_api/post_to_api callers accordingly and apply the same change for the other occurrence around the block that spans the code handling lines 630-647.
393-438:⚠️ Potential issue | 🟠 MajorBound RuneStop retry loops.
This loop can still run forever when a RuneStop hook repeatedly returns
1and the follow-up does not fix the issue. Add a max retry guard before re-running the full hook suite.♻️ Proposed retry guard
+MAX_RUNE_STOP_RETRIES = 5 + async def _run_rune_stop_hooks( @@ - while True: + retry_count = 0 + while True: restarted = False for hook in rune_stop_hooks: @@ if result.returncode == 1: _log_hook("RuneStop", hook.command, 1, hook_output) + retry_count += 1 + if retry_count > MAX_RUNE_STOP_RETRIES: + logger.error( + "RuneStop hook %s still failing after %d retries; giving up", + hook.command, + MAX_RUNE_STOP_RETRIES, + ) + return False, last_ns follow_up = ( "A post-completion hook reported an issue and provided " f"additional context. Please review and address it:\n\n{hook_output}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 393 - 438, The while loop handling rune_stop_hooks can spin forever if hooks keep returning 1; add a bounded retry guard (e.g., introduce a MAX_RUNE_STOP_RESTARTS constant and a restart_count variable) in the block that sets restarted = True so that each time you restart the hook suite you increment restart_count and if it exceeds MAX_RUNE_STOP_RESTARTS you log an error and return False, last_ns; ensure the counter is checked before re-entering the loop (or when deciding to restart) so _run_hook_command, _log_hook, and the follow-up/_drain_messages flow remain unchanged but bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude-orchestrator/agent.py`:
- Around line 185-206: The hook runner _run_hook_command currently blocks
indefinitely; add a timeout to subprocess.run (e.g., a configurable HOOK_TIMEOUT
constant) and handle subprocess.TimeoutExpired so hung RuneStart/RuneStop hooks
don't stall the orchestrator. Modify _run_hook_command to pass
timeout=<HOOK_TIMEOUT> to subprocess.run, wrap the call in try/except
subprocess.TimeoutExpired, and in the except return a
subprocess.CompletedProcess-like result indicating a timeout (non-zero
returncode and stderr explaining timeout, and empty or partial stdout) so
callers (including RuneStop) treat the hook as a failed/blocked result rather
than silently succeeding. Ensure the timeout value is configurable and
referenced where hooks are invoked.
- Around line 230-247: The _build_prompt function currently omits the rune's
acceptance_criteria field; update _build_prompt(rune: dict) to append
acceptance_criteria (using the canonical key "acceptance_criteria") into the
prompt output similar to description/notes/dependencies — e.g., add a blank
line, a header "Acceptance Criteria:", and then either the string content or
iterate if it's a list—so the Claude worker sees it; update or add unit tests
for _build_prompt to assert that acceptance_criteria appears in the returned
string when present (and absent when not).
---
Duplicate comments:
In `@claude-orchestrator/agent.py`:
- Around line 530-535: The post_to_api error handling swallows all exceptions
which breaks the documented contract and makes callers like
append_completion_note_to_api (used in the try/except around
format_completion_note and append_completion_note_to_api) believe a failure
succeeded; update post_to_api to stop catching broad Exception—only catch and
wrap specific transient/network exceptions if you need to add context, then
re-raise them (or raise a new, documented exception type) so callers can handle
failures; adjust append_completion_note_to_api/post_to_api callers accordingly
and apply the same change for the other occurrence around the block that spans
the code handling lines 630-647.
- Around line 393-438: The while loop handling rune_stop_hooks can spin forever
if hooks keep returning 1; add a bounded retry guard (e.g., introduce a
MAX_RUNE_STOP_RESTARTS constant and a restart_count variable) in the block that
sets restarted = True so that each time you restart the hook suite you increment
restart_count and if it exceeds MAX_RUNE_STOP_RESTARTS you log an error and
return False, last_ns; ensure the counter is checked before re-entering the loop
(or when deciding to restart) so _run_hook_command, _log_hook, and the
follow-up/_drain_messages flow remain unchanged but bounded.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a0b3772-7dca-4f6a-a966-dfdfb4c062d6
📒 Files selected for processing (1)
claude-orchestrator/agent.py
Comment on lines
+185
to
+206
| def _run_hook_command( | ||
| command: str, rune_json: str, project_dir: str, last_agent_message: str | None = None | ||
| ) -> subprocess.CompletedProcess: | ||
| """Run a hook command with rune JSON on stdin, using shell for expansion.""" | ||
| env = os.environ.copy() | ||
| env["CLAUDE_PROJECT_DIR"] = project_dir | ||
|
|
||
| # Construct the JSON structure with rune and last_agent_message | ||
| hook_input = json.dumps({ | ||
| "rune": json.loads(rune_json), | ||
| "last_agent_message": last_agent_message | ||
| }) | ||
|
|
||
| return subprocess.run( | ||
| command, | ||
| shell=True, | ||
| input=hook_input, | ||
| capture_output=True, | ||
| text=True, | ||
| env=env, | ||
| cwd=project_dir, | ||
| ) |
Contributor
There was a problem hiding this comment.
Add a timeout for hook commands.
A hung RuneStart/RuneStop hook blocks the entire orchestrator indefinitely. Bound hook execution and convert timeouts into a blocking hook result so RuneStop cannot silently fulfill stale work.
🛡️ Proposed timeout handling
+DEFAULT_HOOK_TIMEOUT_SECONDS = 300
+
def _run_hook_command(
command: str, rune_json: str, project_dir: str, last_agent_message: str | None = None
) -> subprocess.CompletedProcess:
@@
- return subprocess.run(
- command,
- shell=True,
- input=hook_input,
- capture_output=True,
- text=True,
- env=env,
- cwd=project_dir,
- )
+ timeout_s = int(
+ os.environ.get("BIFROST_HOOK_TIMEOUT_SECONDS", DEFAULT_HOOK_TIMEOUT_SECONDS)
+ )
+ try:
+ return subprocess.run(
+ command,
+ shell=True,
+ input=hook_input,
+ capture_output=True,
+ text=True,
+ env=env,
+ cwd=project_dir,
+ timeout=timeout_s,
+ )
+ except subprocess.TimeoutExpired as exc:
+ stdout = exc.stdout if isinstance(exc.stdout, str) else ""
+ stderr = exc.stderr if isinstance(exc.stderr, str) else ""
+ timeout_msg = f"Hook timed out after {timeout_s}s"
+ return subprocess.CompletedProcess(
+ args=command,
+ returncode=2,
+ stdout=stdout,
+ stderr=f"{stderr}\n{timeout_msg}".strip(),
+ )📝 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
| def _run_hook_command( | |
| command: str, rune_json: str, project_dir: str, last_agent_message: str | None = None | |
| ) -> subprocess.CompletedProcess: | |
| """Run a hook command with rune JSON on stdin, using shell for expansion.""" | |
| env = os.environ.copy() | |
| env["CLAUDE_PROJECT_DIR"] = project_dir | |
| # Construct the JSON structure with rune and last_agent_message | |
| hook_input = json.dumps({ | |
| "rune": json.loads(rune_json), | |
| "last_agent_message": last_agent_message | |
| }) | |
| return subprocess.run( | |
| command, | |
| shell=True, | |
| input=hook_input, | |
| capture_output=True, | |
| text=True, | |
| env=env, | |
| cwd=project_dir, | |
| ) | |
| DEFAULT_HOOK_TIMEOUT_SECONDS = 300 | |
| def _run_hook_command( | |
| command: str, rune_json: str, project_dir: str, last_agent_message: str | None = None | |
| ) -> subprocess.CompletedProcess: | |
| """Run a hook command with rune JSON on stdin, using shell for expansion.""" | |
| env = os.environ.copy() | |
| env["CLAUDE_PROJECT_DIR"] = project_dir | |
| # Construct the JSON structure with rune and last_agent_message | |
| hook_input = json.dumps({ | |
| "rune": json.loads(rune_json), | |
| "last_agent_message": last_agent_message | |
| }) | |
| timeout_s = int( | |
| os.environ.get("BIFROST_HOOK_TIMEOUT_SECONDS", DEFAULT_HOOK_TIMEOUT_SECONDS) | |
| ) | |
| try: | |
| return subprocess.run( | |
| command, | |
| shell=True, | |
| input=hook_input, | |
| capture_output=True, | |
| text=True, | |
| env=env, | |
| cwd=project_dir, | |
| timeout=timeout_s, | |
| ) | |
| except subprocess.TimeoutExpired as exc: | |
| stdout = exc.stdout if isinstance(exc.stdout, str) else "" | |
| stderr = exc.stderr if isinstance(exc.stderr, str) else "" | |
| timeout_msg = f"Hook timed out after {timeout_s}s" | |
| return subprocess.CompletedProcess( | |
| args=command, | |
| returncode=2, | |
| stdout=stdout, | |
| stderr=f"{stderr}\n{timeout_msg}".strip(), | |
| ) |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 198-198: subprocess call with shell=True identified, security issue
(S602)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 185 - 206, The hook runner
_run_hook_command currently blocks indefinitely; add a timeout to subprocess.run
(e.g., a configurable HOOK_TIMEOUT constant) and handle
subprocess.TimeoutExpired so hung RuneStart/RuneStop hooks don't stall the
orchestrator. Modify _run_hook_command to pass timeout=<HOOK_TIMEOUT> to
subprocess.run, wrap the call in try/except subprocess.TimeoutExpired, and in
the except return a subprocess.CompletedProcess-like result indicating a timeout
(non-zero returncode and stderr explaining timeout, and empty or partial stdout)
so callers (including RuneStop) treat the hook as a failed/blocked result rather
than silently succeeding. Ensure the timeout value is configurable and
referenced where hooks are invoked.
Comment on lines
+230
to
+247
| def _build_prompt(rune: dict) -> str: | ||
| lines = [ | ||
| f"Rune ID: {rune['id']}", | ||
| f"Title: {rune['title']}", | ||
| ] | ||
| if rune.get("description"): | ||
| lines += ["", "Description:", rune["description"]] | ||
| if rune.get("notes"): | ||
| lines += ["", "Notes:"] | ||
| for note in rune["notes"]: | ||
| if isinstance(note, dict) and note.get("text"): | ||
| lines.append(f" - {note['text']}") | ||
| if rune.get("dependencies"): | ||
| lines += ["", "Dependencies:"] | ||
| for dep in rune["dependencies"]: | ||
| if isinstance(dep, dict): | ||
| lines.append(f" - {dep.get('target_id')} ({dep.get('relationship')})") | ||
| return "\n".join(lines) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the acceptance-criteria field names used by rune payloads.
# Expect: Identify the JSON/domain field that agent.py should render into the prompt.
rg -n -C3 '(acceptance[_-]?criteria|acceptanceCriteria|Acceptance Criteria)'Repository: devzeebo/bifrost
Length of output: 9820
🏁 Script executed:
find . -name "agent.py" -path "*/claude-orchestrator/*" | head -20Repository: devzeebo/bifrost
Length of output: 90
🏁 Script executed:
fd "agent.py" --full-path "*claude-orchestrator*"Repository: devzeebo/bifrost
Length of output: 168
🏁 Script executed:
fd "test.*agent" -e pyRepository: devzeebo/bifrost
Length of output: 105
🏁 Script executed:
cat -n claude-orchestrator/agent.py | head -300Repository: devzeebo/bifrost
Length of output: 10905
🏁 Script executed:
cat -n claude-orchestrator/test_agent_integration.pyRepository: devzeebo/bifrost
Length of output: 19737
Include acceptance_criteria in the worker prompt.
_build_prompt() drops the acceptance_criteria field from the rune payload, preventing the Claude worker from seeing the new acceptance criteria this PR adds. Add the field alongside description, notes, and dependencies, then include corresponding test cases.
The canonical field name is acceptance_criteria (snake_case), confirmed from the domain model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 230 - 247, The _build_prompt
function currently omits the rune's acceptance_criteria field; update
_build_prompt(rune: dict) to append acceptance_criteria (using the canonical key
"acceptance_criteria") into the prompt output similar to
description/notes/dependencies — e.g., add a blank line, a header "Acceptance
Criteria:", and then either the string content or iterate if it's a list—so the
Claude worker sees it; update or add unit tests for _build_prompt to assert that
acceptance_criteria appears in the returned string when present (and absent when
not).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude-orchestrator/agent.py`:
- Around line 469-480: The function _run_agent currently annotates -> bool but
actually returns a 2-tuple; update its signature to return Tuple[bool, bool] (or
tuple[bool, bool]) and add the necessary typing import (from typing import
Tuple) or use built-in tuple typing; then audit all return sites inside
_run_agent (including the early-exit returns that currently return False at
functions referenced as _run_agent and the branches around
hooks_passed/skip_fulfill) and make them return a consistent 2-tuple (e.g.,
(False, False) or the appropriate boolean pair), ensuring the successful path
still returns (hooks_passed, skip_fulfill) and that callers which destructure
the result keep working.
- Around line 174-177: Replace the -2 sentinel with a positive POSIX-safe exit
code constant (e.g. SKIP_FULFILL_EXIT = 75) and use it everywhere: change
sys.exit(-2) to sys.exit(SKIP_FULFILL_EXIT), update all subprocess returncode
checks that currently test result.returncode == -2 to compare against
SKIP_FULFILL_EXIT, and align hook scripts, dispatcher.py, and cli/orchestrate.go
to use the same constant; also fix the incorrect return annotation (the function
that currently declares -> bool but returns (hooks_passed, skip_fulfill)) to the
correct tuple type (e.g. -> Tuple[bool, bool] or tuple[bool, bool]) and import
typing as needed so the signature matches the actual return value.
In `@cli/orchestrate.go`:
- Around line 286-291: The skip-fulfill sentinel -2 is unreachable on POSIX;
change the sentinel to a documented in-range positive value (e.g., 75) and
update the exit-code checks accordingly: replace the check in orchestrate.go
that tests exitCode == -2 with exitCode == 75 (keeping the unclaimRune(client,
id) call and return), and make the matching change in agent.py (where
sys.exit(-2) is emitted) and any dispatcher logic that interprets the sentinel
so all three components use the same positive sentinel value.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 712c5a79-53e5-411e-afbf-450ea1b06346
📒 Files selected for processing (2)
claude-orchestrator/agent.pycli/orchestrate.go
Comment on lines
+174
to
+177
| # If RuneStop hook said skip fulfill (-2), exit -2 | ||
| if skip_fulfill: | ||
| logger.info("RuneStop hook signaled skip fulfill (-2), exiting with -2") | ||
| sys.exit(-2) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Empirically demonstrate POSIX 8-bit truncation and subprocess.returncode semantics.
python3 -c 'import sys; sys.exit(-2)'; echo "shell saw: $?"
python3 - <<'PY'
import subprocess
r = subprocess.run(["python3", "-c", "import sys; sys.exit(-2)"])
print("returncode seen by parent Python:", r.returncode)
PY
# Also check whether any existing hook scripts or dispatcher already use -2 vs 254.
rg -nP -C2 '\b(returncode|exit(code|_code)?)\s*==\s*-2\b'Repository: devzeebo/bifrost
Length of output: 805
🏁 Script executed:
# Check exact code at the cited lines
sed -n '174,177p' claude-orchestrator/agent.py
sed -n '237,241p' claude-orchestrator/agent.py
sed -n '429,433p' claude-orchestrator/agent.py
sed -n '478,482p' claude-orchestrator/agent.pyRepository: devzeebo/bifrost
Length of output: 796
🏁 Script executed:
# Check if cli/orchestrate.go exists and what it checks for
find . -name "orchestrate.go" -type f -exec cat {} \; | head -100Repository: devzeebo/bifrost
Length of output: 2919
🏁 Script executed:
# Verify return type annotation at line 480
ast-grep --pattern 'def $_($_) -> $_:' | head -20Repository: devzeebo/bifrost
Length of output: 42
🏁 Script executed:
# Get the full function signature starting around line 480
sed -n '460,490p' claude-orchestrator/agent.pyRepository: devzeebo/bifrost
Length of output: 877
🏁 Script executed:
# Find the return statement(s) in the function marked with -> bool
# Line 480 has -> bool, let's find what comes after line 500 for returns
sed -n '550,570p' claude-orchestrator/agent.pyRepository: devzeebo/bifrost
Length of output: 770
🏁 Script executed:
# Search for all return statements in the _run_agent function
rg -n 'return.*skip_fulfill' claude-orchestrator/agent.py -A2 -B2Repository: devzeebo/bifrost
Length of output: 772
🏁 Script executed:
# Check if cli/orchestrate.go actually processes the exit code -2 or 254
rg -n 'exit.*(-2|254)' . --iglob '*.go'Repository: devzeebo/bifrost
Length of output: 105
The -2 exit-code / returncode convention is broken on POSIX and will never work as intended.
Three places assume a -2 sentinel can flow through a process boundary, but it cannot on Linux/macOS:
-
Line 177 —
sys.exit(-2): POSIXwait()returns only the low 8 bits of the argument to_exit(2), so this emits exit status 254, not-2. The Go caller'sif exitCode == -2check incli/orchestrate.go:287therefore never matches and skip-fulfill silently falls into the generic failure path. Python's own docs note "Most systems require it to be in the range 0–127, and produce undefined results otherwise", and in POSIX "the older wait() and waitpid() calls retrieve only the least significant 8 bits of the exit status". -
Line 239 —
if result.returncode == -2:for RuneStart hooks and line 431 — same check for RuneStop hooks:subprocess.CompletedProcess.returncodeis negative only when a child was killed by a signal (value is-signal), never for a negative exit argument. A hook script that doesexit -2in bash (orsys.exit(-2)in Python) will be reported here asreturncode == 254, not-2. So these branches are effectively dead.
Pick a positive sentinel in-range (e.g. 75) and document it as the "skip / skip-fulfill" code. Align agent.py, hook scripts, dispatcher.py, and cli/orchestrate.go on the same value.
🛠 Suggested direction
+SKIP_FULFILL_EXIT_CODE = 75 # keep in sync with cli/orchestrate.go
+
...
- if skip_fulfill:
- logger.info("RuneStop hook signaled skip fulfill (-2), exiting with -2")
- sys.exit(-2)
+ if skip_fulfill:
+ logger.info("RuneStop hook signaled skip fulfill, exiting with %d", SKIP_FULFILL_EXIT_CODE)
+ sys.exit(SKIP_FULFILL_EXIT_CODE)
...
- if result.returncode == -2:
+ if result.returncode == SKIP_FULFILL_EXIT_CODE:Also, the return annotation at line 480 declares -> bool but the function returns a tuple (hooks_passed, skip_fulfill) — fix to match the actual return type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 174 - 177, Replace the -2 sentinel
with a positive POSIX-safe exit code constant (e.g. SKIP_FULFILL_EXIT = 75) and
use it everywhere: change sys.exit(-2) to sys.exit(SKIP_FULFILL_EXIT), update
all subprocess returncode checks that currently test result.returncode == -2 to
compare against SKIP_FULFILL_EXIT, and align hook scripts, dispatcher.py, and
cli/orchestrate.go to use the same constant; also fix the incorrect return
annotation (the function that currently declares -> bool but returns
(hooks_passed, skip_fulfill)) to the correct tuple type (e.g. -> Tuple[bool,
bool] or tuple[bool, bool]) and import typing as needed so the signature matches
the actual return value.
Comment on lines
+286
to
+291
| // Exit -2 = success but skip fulfill (unclaim only) | ||
| if exitCode == -2 { | ||
| fmt.Fprintf(os.Stderr, "orchestrate: [%s] agent signaled skip-fulfill (-2), unclaiming\n", id) | ||
| unclaimRune(client, id) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Locate RunDispatched implementation to see how exitCode is obtained.
rg -nP -C5 '\bfunc\s+RunDispatched\s*\('
# 2. Check dispatcher.py for any handling of -2 vs 254.
rg -nP -C3 '\b(-2|254|skip[-_]?fulfill)\b' claude-orchestrator/
# 3. Demonstrate POSIX 8-bit truncation empirically.
python3 -c 'import sys; sys.exit(-2)'; echo "observed: $?"Repository: devzeebo/bifrost
Length of output: 50372
🏁 Script executed:
cat -n cli/orchestrate_exec.go | head -50Repository: devzeebo/bifrost
Length of output: 1172
🏁 Script executed:
sed -n '155,180p' claude-orchestrator/agent.pyRepository: devzeebo/bifrost
Length of output: 618
🏁 Script executed:
sed -n '280,305p' cli/orchestrate.goRepository: devzeebo/bifrost
Length of output: 644
exitCode == -2 branch is unreachable on POSIX — skip-fulfill signaling is broken end-to-end.
On Unix, child process exit status is truncated to the least-significant 8 bits (WEXITSTATUS). When claude-orchestrator/agent.py does sys.exit(-2) (line 177), the parent will observe exit status 254, not -2. Go's exec.Cmd.ProcessState.ExitCode() returns that OS-level status, so the if exitCode == -2 check on line 286 will never fire. The orchestrator instead falls through to the exitCode != 0 branch (line 294), treating skip-fulfill as a generic failure and potentially honoring unclaimOnFailure rather than unconditionally unclaiming.
Pick an in-range sentinel (e.g., a documented positive code like 75, kept distinct from 0/1/2 already used by hooks) and align the agent, dispatcher, and this file on it. Root cause is in claude-orchestrator/agent.py; matching change required there too.
🛠 Suggested direction (requires matching change in agent.py)
- // Exit -2 = success but skip fulfill (unclaim only)
- if exitCode == -2 {
+ // Exit SkipFulfillExitCode = success but skip fulfill (unclaim only)
+ const SkipFulfillExitCode = 75 // keep in sync with agent.py
+ if exitCode == SkipFulfillExitCode {
fmt.Fprintf(os.Stderr, "orchestrate: [%s] agent signaled skip-fulfill (-2), unclaiming\n", id)
unclaimRune(client, id)
return
}📝 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
| // Exit -2 = success but skip fulfill (unclaim only) | |
| if exitCode == -2 { | |
| fmt.Fprintf(os.Stderr, "orchestrate: [%s] agent signaled skip-fulfill (-2), unclaiming\n", id) | |
| unclaimRune(client, id) | |
| return | |
| } | |
| // Exit SkipFulfillExitCode = success but skip fulfill (unclaim only) | |
| const SkipFulfillExitCode = 75 // keep in sync with agent.py | |
| if exitCode == SkipFulfillExitCode { | |
| fmt.Fprintf(os.Stderr, "orchestrate: [%s] agent signaled skip-fulfill (-2), unclaiming\n", id) | |
| unclaimRune(client, id) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/orchestrate.go` around lines 286 - 291, The skip-fulfill sentinel -2 is
unreachable on POSIX; change the sentinel to a documented in-range positive
value (e.g., 75) and update the exit-code checks accordingly: replace the check
in orchestrate.go that tests exitCode == -2 with exitCode == 75 (keeping the
unclaimRune(client, id) call and return), and make the matching change in
agent.py (where sys.exit(-2) is emitted) and any dispatcher logic that
interprets the sentinel so all three components use the same positive sentinel
value.
- Add ProjectDir field to DispatchInput struct (JSON key: "cwd") - Populate cwd from os.Getwd() in dispatchInputFromRune() - Extract cwd from rune in agent.py, fallback to _find_project_root() - Pass cwd to hooks alongside rune + last_agent_message - Fix test assertions for _run_agent tuple return (success, skip_fulfill) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- _run_rune_start_hooks now returns (output, skip_agent, error) tuple - Any hook returning positive error (1, 2, etc.) signals failure - Agent exits with code 1 if any RuneStart hook fails - Preserves existing behavior for exit -2 (skip) and exit 0 (success) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
When a RuneStart hook returns a positive error code, log both stdout and stderr in full (unredacted) to help with debugging. Previously truncated to 100 chars in the structured log line. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add raw_detail field to DispatchInput containing the complete rune map from bf show, so hooks receive the full data structure instead of a rebuilt subset of fields. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
claude-orchestrator/agent.py (6)
687-697:⚠️ Potential issue | 🟠 Major
post_to_apicontract and behavior are inconsistent.Docstring says it raises, but implementation swallows exceptions; callers cannot reliably react to API failures.
Suggested fix
- try: - response = requests.post(url, json=payload, timeout=30) - response.raise_for_status() - except Exception as exc: - logger.warning("Failed to POST to %s: %s", url, exc) + response = requests.post(url, json=payload, timeout=30) + response.raise_for_status()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 687 - 697, The implementation of post_to_api swallows exceptions from requests.post despite the docstring stating it raises; update the try/except in post_to_api so it either re-raises the caught exception (after logging via logger.warning) or wraps and raises a clearer custom exception so callers can handle failures; locate the requests.post call and the except Exception as exc block in post_to_api and change it to logger.warning(...); raise (or raise CustomApiError(...) from exc) after logging to make behavior match the docstring and allow callers to react to API/network errors.
501-503:⚠️ Potential issue | 🟠 MajorFix
_run_agentreturn type annotation to tuple contract.Signature says
-> boolbut returns(bool, bool)at multiple paths.Suggested fix
-) -> bool: +) -> tuple[bool, bool]:Also applies to: 538-539, 547-548, 562-563, 587-587
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 501 - 503, The _run_agent function's signature currently annotates a bool return but actually returns a pair of booleans in multiple places; update the return type to a tuple type (e.g., Tuple[bool, bool]) and import the typing symbol if needed, then ensure every return in _run_agent (and any overloaded/related returns called at lines referenced: the branches around lines 538-539, 547-548, 562-563, 587) matches that contract; adjust callers if they expect a single bool (or unpack the tuple) so that uses of _run_agent and any references to its results handle the (fulfilled, stop_hooks_run) tuple consistently.
272-289:⚠️ Potential issue | 🟠 MajorInclude
acceptance_criteriain the generated worker prompt.The prompt builder currently omits AC, so agents won’t see the criteria this PR introduces.
Suggested fix
if rune.get("dependencies"): lines += ["", "Dependencies:"] for dep in rune["dependencies"]: if isinstance(dep, dict): lines.append(f" - {dep.get('target_id')} ({dep.get('relationship')})") + if rune.get("acceptance_criteria"): + lines += ["", "Acceptance Criteria:"] + ac = rune["acceptance_criteria"] + if isinstance(ac, list): + for item in ac: + lines.append(f" - {item}") + else: + lines.append(str(ac))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 272 - 289, The prompt builder _build_prompt currently omits the rune's acceptance_criteria so agents never see the AC; update _build_prompt to append an "Acceptance Criteria:" section when rune.get("acceptance_criteria") is present—handle both string and list forms (if it's a string, append it directly; if it's a list, iterate and for each item append a bullet, and if an item is a dict use item.get('text')). Ensure the new section is added like the existing Description/Notes blocks and include the section header and formatted lines before returning the joined prompt.
180-183:⚠️ Potential issue | 🔴 CriticalReplace
-2sentinel with a POSIX-safe positive exit code.
sys.exit(-2)andresult.returncode == -2are not portable through process boundaries on POSIX; this skip path will not behave as intended.Suggested fix
+SKIP_EXIT_CODE = 75 # keep in sync with dispatcher/CLI checks ... - if skip_fulfill: - logger.info("RuneStop hook signaled skip fulfill (-2), exiting with -2") - sys.exit(-2) + if skip_fulfill: + logger.info("RuneStop hook signaled skip fulfill, exiting with %d", SKIP_EXIT_CODE) + sys.exit(SKIP_EXIT_CODE) ... - if result.returncode == -2: + if result.returncode == SKIP_EXIT_CODE: ... - if result.returncode == -2: - _log_hook("RuneStop", hook.command, -2, hook_output) + if result.returncode == SKIP_EXIT_CODE: + _log_hook("RuneStop", hook.command, SKIP_EXIT_CODE, hook_output)Also applies to: 248-249, 452-453
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 180 - 183, The code uses a negative sentinel sys.exit(-2) and checks for result.returncode == -2 which is not POSIX-safe; replace the negative sentinel with a positive exit code constant (e.g. define SKIP_FULFILL_EXIT_CODE = 2) and use that constant everywhere: change sys.exit(-2) calls (e.g. in the skip_fulfill branch where logger.info is called) to sys.exit(SKIP_FULFILL_EXIT_CODE) and update any result.returncode == -2 checks to compare against SKIP_FULFILL_EXIT_CODE (also update the other occurrences of sys.exit(-2)/returncode checks in this file).
437-487:⚠️ Potential issue | 🟠 MajorBound RuneStop retry loops to prevent infinite churn.
while True+ restart-on-1 has no max retry guard and can run forever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 437 - 487, The infinite retry risk in the RuneStop loop (the while True that iterates rune_stop_hooks and uses restarted when a hook returns code 1) must be bounded: add a max-retry counter (e.g. MAX_RUNE_STOP_RETRIES) and a local retry_count that increments each time you set restarted=True (or before breaking to retry), check the counter before restarting and if it exceeds the max log an error via logger.error and return False, last_ns, skip_fulfill; ensure the loop condition uses the counter instead of while True and keep uses of _run_hook_command, _log_hook, client.query and _drain_messages unchanged except for the added guard and error log when the retry limit is hit.
215-223:⚠️ Potential issue | 🟠 MajorAdd a timeout to hook command execution.
A hung hook can block the orchestrator indefinitely.
Suggested fix
+DEFAULT_HOOK_TIMEOUT_SECONDS = 300 ... - return subprocess.run( - command, - shell=True, - input=hook_input, - capture_output=True, - text=True, - env=env, - cwd=project_dir, - ) + timeout_s = int(os.environ.get("BIFROST_HOOK_TIMEOUT_SECONDS", DEFAULT_HOOK_TIMEOUT_SECONDS)) + try: + return subprocess.run( + command, + shell=True, + input=hook_input, + capture_output=True, + text=True, + env=env, + cwd=project_dir, + timeout=timeout_s, + ) + except subprocess.TimeoutExpired as exc: + return subprocess.CompletedProcess( + args=command, + returncode=2, + stdout=exc.stdout if isinstance(exc.stdout, str) else "", + stderr=((exc.stderr if isinstance(exc.stderr, str) else "") + f"\nHook timed out after {timeout_s}s").strip(), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/agent.py` around lines 215 - 223, The subprocess.run call that executes hooks can hang indefinitely; add a timeout to it and handle subprocess.TimeoutExpired: pass a configurable timeout value (e.g., from an env var or function parameter like hook_timeout) into the subprocess.run(...) call where command, hook_input, env, and project_dir are used, wrap the call in a try/except subprocess.TimeoutExpired block, and in the except branch log the timeout (including command and timeout value) and return or synthesize a non-zero result (or raise a clear exception) so the orchestrator doesn't block forever.claude-orchestrator/test_agent_integration.py (1)
102-104:⚠️ Potential issue | 🟠 MajorRemove always-true assertions (
or True).These checks can never fail, so they don’t validate behavior.
Suggested fix
- assert ( - mock_post.called or True - ) # Temporary allowance for test to fail properly + assert mock_post.called ... - assert mock_post.called or True + assert mock_post.called ... - assert mock_post.called or True + assert mock_post.calledAlso applies to: 357-357, 476-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude-orchestrator/test_agent_integration.py` around lines 102 - 104, Replace the no-op assertions that append "or True" with real checks on the mock; locate the three assertions using "mock_post.called or True" (instances where mock_post.called is being asserted) and remove the "or True" so they read e.g. "assert mock_post.called" or, if you need a stronger check, assert "mock_post.call_count > 0"; ensure each occurrence (the three places using mock_post.called) is updated to a meaningful assertion rather than an always-true expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude-orchestrator/agent.py`:
- Around line 133-135: Use the incoming rune's DispatchInput.ProjectDir value as
the cwd before falling back to project-root discovery: replace the unconditional
call to _find_project_root() in agent.py with a lookup of
rune.get(DispatchInput.ProjectDir) (or equivalent key on the rune) and set cwd
to that if present, otherwise call _find_project_root(); keep rune_id assignment
unchanged. Ensure you import or reference the DispatchInput symbol where used
and handle missing/empty values the same way as the original fallback.
In `@claude-orchestrator/test_agent_integration.py`:
- Around line 233-240: The test currently swallows all exceptions with a bare
try/except/pass around append_completion_note_to_api(rune_id, note_text,
"http://localhost:8000"), making the test always pass; replace the silent catch
with explicit assertions: either assert the function returns the expected result
when successful, or use pytest.raises to assert a specific exception
type/message when an error is expected, and verify side effects (e.g., API call
occurred or note persisted) where applicable so failures surface; locate the
call to append_completion_note_to_api in test_agent_integration.py and update
the surrounding block to assert the correct outcome instead of pass.
- Around line 144-148: The test's current check using mock_post.call_args_list
only ensures "/add-note" was not used but doesn't assert that no other
unexpected endpoints were called; modify the assertion to directly verify that
post_to_api was not invoked (i.e., mock_post.assert_not_called()) or, if other
posts are allowed, iterate mock_post.call_args_list and assert each call's URL
is exactly the expected one and not any other endpoint (check call arg that
contains the endpoint string). Update the test around mock_post usage in
test_agent_integration.py to replace the loose loop with a direct
mock_post.assert_not_called() when no posts should occur, or with explicit
equality checks on each call's first positional or named 'url' argument to
reject any endpoint like "/add-note".
---
Duplicate comments:
In `@claude-orchestrator/agent.py`:
- Around line 687-697: The implementation of post_to_api swallows exceptions
from requests.post despite the docstring stating it raises; update the
try/except in post_to_api so it either re-raises the caught exception (after
logging via logger.warning) or wraps and raises a clearer custom exception so
callers can handle failures; locate the requests.post call and the except
Exception as exc block in post_to_api and change it to logger.warning(...);
raise (or raise CustomApiError(...) from exc) after logging to make behavior
match the docstring and allow callers to react to API/network errors.
- Around line 501-503: The _run_agent function's signature currently annotates a
bool return but actually returns a pair of booleans in multiple places; update
the return type to a tuple type (e.g., Tuple[bool, bool]) and import the typing
symbol if needed, then ensure every return in _run_agent (and any
overloaded/related returns called at lines referenced: the branches around lines
538-539, 547-548, 562-563, 587) matches that contract; adjust callers if they
expect a single bool (or unpack the tuple) so that uses of _run_agent and any
references to its results handle the (fulfilled, stop_hooks_run) tuple
consistently.
- Around line 272-289: The prompt builder _build_prompt currently omits the
rune's acceptance_criteria so agents never see the AC; update _build_prompt to
append an "Acceptance Criteria:" section when rune.get("acceptance_criteria") is
present—handle both string and list forms (if it's a string, append it directly;
if it's a list, iterate and for each item append a bullet, and if an item is a
dict use item.get('text')). Ensure the new section is added like the existing
Description/Notes blocks and include the section header and formatted lines
before returning the joined prompt.
- Around line 180-183: The code uses a negative sentinel sys.exit(-2) and checks
for result.returncode == -2 which is not POSIX-safe; replace the negative
sentinel with a positive exit code constant (e.g. define SKIP_FULFILL_EXIT_CODE
= 2) and use that constant everywhere: change sys.exit(-2) calls (e.g. in the
skip_fulfill branch where logger.info is called) to
sys.exit(SKIP_FULFILL_EXIT_CODE) and update any result.returncode == -2 checks
to compare against SKIP_FULFILL_EXIT_CODE (also update the other occurrences of
sys.exit(-2)/returncode checks in this file).
- Around line 437-487: The infinite retry risk in the RuneStop loop (the while
True that iterates rune_stop_hooks and uses restarted when a hook returns code
1) must be bounded: add a max-retry counter (e.g. MAX_RUNE_STOP_RETRIES) and a
local retry_count that increments each time you set restarted=True (or before
breaking to retry), check the counter before restarting and if it exceeds the
max log an error via logger.error and return False, last_ns, skip_fulfill;
ensure the loop condition uses the counter instead of while True and keep uses
of _run_hook_command, _log_hook, client.query and _drain_messages unchanged
except for the added guard and error log when the retry limit is hit.
- Around line 215-223: The subprocess.run call that executes hooks can hang
indefinitely; add a timeout to it and handle subprocess.TimeoutExpired: pass a
configurable timeout value (e.g., from an env var or function parameter like
hook_timeout) into the subprocess.run(...) call where command, hook_input, env,
and project_dir are used, wrap the call in a try/except
subprocess.TimeoutExpired block, and in the except branch log the timeout
(including command and timeout value) and return or synthesize a non-zero result
(or raise a clear exception) so the orchestrator doesn't block forever.
In `@claude-orchestrator/test_agent_integration.py`:
- Around line 102-104: Replace the no-op assertions that append "or True" with
real checks on the mock; locate the three assertions using "mock_post.called or
True" (instances where mock_post.called is being asserted) and remove the "or
True" so they read e.g. "assert mock_post.called" or, if you need a stronger
check, assert "mock_post.call_count > 0"; ensure each occurrence (the three
places using mock_post.called) is updated to a meaningful assertion rather than
an always-true expression.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ceb2972e-0b31-41f4-a2a4-4dd96477434e
📒 Files selected for processing (3)
claude-orchestrator/agent.pyclaude-orchestrator/test_agent_integration.pycli/orchestrate_dispatch.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/orchestrate_dispatch.go
Comment on lines
+133
to
+135
| # Extract cwd from rune if available, otherwise fall back to finding project root | ||
| cwd = _find_project_root() | ||
| rune_id = rune.get("id", "unknown") |
Contributor
There was a problem hiding this comment.
Use DispatchInput.ProjectDir before falling back to project-root discovery.
Line 134 ignores the incoming rune working directory, which breaks the PR objective to propagate caller cwd into hooks/agent execution.
Suggested fix
- cwd = _find_project_root()
+ cwd = (
+ rune.get("project_dir")
+ or rune.get("cwd")
+ or _find_project_root()
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/agent.py` around lines 133 - 135, Use the incoming rune's
DispatchInput.ProjectDir value as the cwd before falling back to project-root
discovery: replace the unconditional call to _find_project_root() in agent.py
with a lookup of rune.get(DispatchInput.ProjectDir) (or equivalent key on the
rune) and set cwd to that if present, otherwise call _find_project_root(); keep
rune_id assignment unchanged. Ensure you import or reference the DispatchInput
symbol where used and handle missing/empty values the same way as the original
fallback.
Comment on lines
+144
to
+148
| if mock_post.called: | ||
| # Check that add-note was NOT called | ||
| for call in mock_post.call_args_list: | ||
| assert "/add-note" not in str(call) | ||
|
|
Contributor
There was a problem hiding this comment.
Assert failure-path post behavior directly.
This branch currently does not fail if post_to_api is unexpectedly called with another endpoint.
Suggested fix
- if mock_post.called:
- # Check that add-note was NOT called
- for call in mock_post.call_args_list:
- assert "/add-note" not in str(call)
+ mock_post.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/test_agent_integration.py` around lines 144 - 148, The
test's current check using mock_post.call_args_list only ensures "/add-note" was
not used but doesn't assert that no other unexpected endpoints were called;
modify the assertion to directly verify that post_to_api was not invoked (i.e.,
mock_post.assert_not_called()) or, if other posts are allowed, iterate
mock_post.call_args_list and assert each call's URL is exactly the expected one
and not any other endpoint (check call arg that contains the endpoint string).
Update the test around mock_post usage in test_agent_integration.py to replace
the loose loop with a direct mock_post.assert_not_called() when no posts should
occur, or with explicit equality checks on each call's first positional or named
'url' argument to reject any endpoint like "/add-note".
Comment on lines
+233
to
+240
| try: | ||
| append_completion_note_to_api( | ||
| rune_id, note_text, "http://localhost:8000" | ||
| ) | ||
| # If it doesn't raise, that's expected (graceful error handling) | ||
| except Exception: | ||
| # If it does raise, that's also acceptable for now | ||
| pass |
Contributor
There was a problem hiding this comment.
Avoid try/except/pass in tests without assertions.
This test currently passes regardless of behavior and won’t catch regressions.
🧰 Tools
🪛 Ruff (0.15.10)
[error] 238-240: try-except-pass detected, consider logging the exception
(S110)
[warning] 238-238: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude-orchestrator/test_agent_integration.py` around lines 233 - 240, The
test currently swallows all exceptions with a bare try/except/pass around
append_completion_note_to_api(rune_id, note_text, "http://localhost:8000"),
making the test always pass; replace the silent catch with explicit assertions:
either assert the function returns the expected result when successful, or use
pytest.raises to assert a specific exception type/message when an error is
expected, and verify side effects (e.g., API call occurred or note persisted)
where applicable so failures surface; locate the call to
append_completion_note_to_api in test_agent_integration.py and update the
surrounding block to assert the correct outcome instead of pass.
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
rune_ac_counter) and detail projection updatescreate,show,updatewith AC fieldsTest plan
make testpasses all modulesmake lintcleanbf showoutput