Fixes to demo made with Felipe - #88
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
Portfolio workflow indexed a Future instead of blocking on its value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Is this ready ? |
Place generated agent stubs both flat and at their entrypoint-mirrored path in agent and workflow Docker contexts, so both 'from price_agent import PriceAgent' and 'from agents.price_agent import PriceAgent' style peer imports resolve. _stub_destination() only ever wrote one destination, contradicting the two comments in cli.py that already claimed dual placement -- this is why MetricsAgent's container logged 'No module named price_agent'. Updated the one test that asserted the old (wrong) single-destination behavior. This same class of bug and fix (ac9a75e, 01a70f2, 137e1db, 0b9546c) has recurred across several unmerged branches; none reached main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6081c87 to
9f86d75
Compare
- Merge cli/README.md's install/command/usage docs into root README.md, correct stale onboarding (new-app command name, workflow vs dashboard port) and add the global_controller.yaml config walkthrough. - Trim cli/README.md to a short pointer at ARCHITECTURE.md. - Un-embed the accidentally nested examples/repo git repo (stale gitlink in the index, no actual .git left on disk) from a prior state -- kept out of scope here, that lives on docs/fleshing-docs-clean. - Assorted small fixes across cli/canyonos/ (constants, deploy, test, theme, init, quit, stop, verify, dashboard_stack, dashboard.compose.yml) and their tests. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
_load_agent derived the module name for spec_from_file_location by stripping .py from the entrypoint path, leaving directory separators intact (e.g. "agents/aml_agent"). Without dots, Python can't establish __package__, so any relative import inside a nested entrypoint (e.g. `from .prompts import PROMPT`) fails with "attempted relative import with no known parent package" at agent load, surfacing as "No agent loaded" at serve time. Convert path separators to dots so __package__ resolves correctly and sibling relative imports work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_assign_new_project_id() minted uuid.uuid4().hex (32-char, no dashes). Dashboard api's bootstrap_canyonos() requires a dashed UUID (UUID_RE / z.string().uuid()) and silently bails with 'CanyonOS identity is unusable, bootstrapping no project' when it doesn't match, leaving the projects table empty and the dashboard UI blank despite a healthy serve stack and spans flowing end-to-end. Fixed by switching to str(uuid.uuid4()). Updated test_global_controller_project_id.py's UUID_HEX_RE to match the dashed format, consistent with test_global_controller_identity.py and test_global_controller_reload.py, which already used dashed fixtures. This exact fix has been written and lost multiple times across unmerged branches (CAN-316-canyonos-serve, docs/fleshing-docs) without ever landing on main.
_is_light_background() gave the terminal only 100ms to answer the OSC 11
background-color query before restoring cooked+echo tty mode. Fine for a
local terminal, but a real SSH round-trip (e.g. to an EC2 box) can exceed
that, so the reply arrives after echo is back on and gets displayed as
literal text ahead of the next command ("^[]11;rgb:.../ ^[\").
Widened the timeout to 400ms and, more importantly, drain any bytes still
pending on the fd (right before restoring termios) so a late or duplicate
reply is swallowed instead of leaking into the shell.
…eploy failure _ERROR_MARKERS' naive "ERROR:" substring match caught ERROR:opentelemetry.exporter.otlp.proto.http.trace_exporter:Failed to export span batch due to timeout... -- which fires on every cold deploy, since the dashboard (the OTel destination) hasn't been started yet at that point in the sequence and the exporter is just retrying as designed. That false positive broke the tail loop before it ever reached the 'Global controller started' success marker, so _start_dashboard() (canyonos serve, bundled into deploy) never got called even though the deploy had fully succeeded. Added _BENIGN_ERROR_PREFIXES, checked before _ERROR_MARKERS, so this logger's own ERROR: lines are treated as non-fatal noise instead.
…-Fixes # Conflicts: # cli/canyonos/constants.py # cli/canyonos/deploy.py # cli/canyonos/theme.py
| e, | ||
| len(spans), | ||
| ) | ||
| elif recorder is not None and recorder.rejected_spans: |
There was a problem hiding this comment.
Could we avoid retrying the whole batch here? The current test expects this incorrect behavior, but OTLP says partial-success responses must not be retried. Could we fix the test first, then update the implementation? Otherwise, the test protects code that can duplicate accepted spans.
| if ( | ||
| isinstance(value, str) | ||
| and len(value) == 32 | ||
| and len(value) == 16 |
There was a problem hiding this comment.
Any normal 16-character hex value is treated as a Future now and can wait for minutes, could we add the failing test first with both a real Future ID and a normal hex string?
| intent = intent_agent.parse(query=query) | ||
| # parse() returns a Future in deployment -- .value() blocks for the result, | ||
| # which comes back as a JSON string like the other stage calls below. | ||
| intent = json.loads(intent_agent.parse(query=query).value()) |
There was a problem hiding this comment.
add a regression test for this P0 before merging, this was the exact line that broke every portfolio request. Without a test that makes parse return a Future, the same bug can come back.
| mark_sent_many.assert_called_once_with(["0011223344556677"], self.db_path) | ||
|
|
||
| def test_send_pending_attempts_remaining_processors_and_leaves_row_unsent_on_failure(self): | ||
| def test_send_pending_leaves_rows_unsent_when_a_destination_returns_failure(self): |
There was a problem hiding this comment.
Could we cover this with an integration test using a real HTTP endpoint that fails and then recovers? Mocking export only proves our branch logic; it does not prove the SDK request, retry, and database acknowledgement work together.
I'm not sure if the repo have e2e tests setup already
| flat_path = Path(output_dir) / "split_agent.py" | ||
| self.assertIn("class SplitAgent", nested_path.read_text()) | ||
| self.assertFalse(flat_path.exists(), "stub must not be duplicated flat") | ||
| self.assertIn("class SplitAgent", flat_path.read_text()) |
There was a problem hiding this comment.
Could we add the same test for generate_docker? Finding #5 happened inside MetricsAgent, but this test only covers the workflow image. The test should reproduce the exact agent-container import that failed
userAugustos
left a comment
userAugustos
left a comment
There was a problem hiding this comment.
In general I would create more/improve the tests, so we don't get to break this again in the future.
4 participants
partially close some issues from CAN-334