Skip to content

refactor(routes): move note domain into routes/note/ subpackage#5236

Merged
RaresKeY merged 2 commits into
odysseus-dev:devfrom
ydonghao:refactor/routes-note-to-subdir
Jul 20, 2026
Merged

refactor(routes): move note domain into routes/note/ subpackage#5236
RaresKeY merged 2 commits into
odysseus-dev:devfrom
ydonghao:refactor/routes-note-to-subdir

Conversation

@ydonghao

@ydonghao ydonghao commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Slice 2f of the route-domain reorganization (Refs #4082 / #4071; inventory baseline in #4148, specs/architecture-runtime-inventory.md §6.3). Moves the note domain out of the flat routes/ directory into a new routes/note/ subpackage, keeping the flat routes/ directory intact for all other domains.

Follows the same pattern as the merged slices: gallery (#4903), research (#4975), memory (#5007), history (#5090), contacts (#5227). No behavior change. Pure file reorganization.

What moves

From To
routes/note_routes.py (915 lines) routes/note/note_routes.py

New routes/note/__init__.py marks the subpackage. Note is a single-file domain (§4) with zero internal routes/ coupling at module top (imports only core/, src/, stdlib). Two function-local lazy imports from routes.email_* (for email-based reminders) keep resolving through the email module's own path — unaffected by this migration.

Backward compatibility

A backward-compat shim remains at routes/note_routes.py. It uses sys.modules replacement (same pattern as all previous slices) so the legacy import path and the canonical path resolve to the same module object:

from routes.note import note_routes as _canonical
sys.modules[__name__] = _canonical

This is required because test_note_reminder_fire_scope.py and test_notes_fail_closed_auth.py use import routes.note_routes as note_routes followed by monkeypatch.setattr(note_routes, "SessionLocal", ...) — for those patches to take effect at runtime, the legacy module object and the canonical one must be identical.

The canonical module does not depend on the shim — routes/note/note_routes.py imports only from core/, src/, and stdlib.

Test repoints (1 site)

One source-introspection test reads the note source by file path. Repointed from routes/note_routes.pyroutes/note/note_routes.py:

Test Site
test_model_helper_owner_scope.py _function_source path arg (shared with history; history entry already repointed in #5090)

Shim regression test

Adds tests/test_note_routes_shim.py to pin the sys.modules shim contract:

  • legacy routes.note_routes and canonical routes.note.note_routes resolve to the same module object
  • monkeypatch via the legacy alias reaches the canonical module

How to test

python -m compileall routes/note/ routes/note_routes.py app.py
python -m pytest tests/ -q

Full suite: 4487 passed, 3 skipped, 0 failed.

Target branch

  • This PR targets dev, not main.

Linked Issue

Follows the Phase 0 architecture refactor (#4082 / #4071) and the merged runtime inventory (#4148). One domain per PR per maintainer guidance.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Refactor / cleanup
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors
  • No runtime behavior changes (pure file move + import shim; verified by full pytest suite)
  • Backward-compatible: old from routes.note_routes import ... paths keep working via shim

Slice 2f of the route-domain reorganization (odysseus-dev#4082/odysseus-dev#4071, per
specs/architecture-runtime-inventory.md §6.3). Moves note_routes.py into
routes/note/, leaving a backward-compat sys.modules shim at the old path.
Pure file reorganization, no behavior change.

The shim uses sys.modules replacement (same pattern as the merged gallery
odysseus-dev#4903, research odysseus-dev#4975, memory odysseus-dev#5007, history odysseus-dev#5090, and contacts odysseus-dev#5227
slices) so that `import routes.note_routes`, `from routes.note_routes import
X`, `importlib.import_module(...)`, and the `import ... as note_routes` +
`monkeypatch.setattr(note_routes, "SessionLocal", ...)` pattern used by
test_note_reminder_fire_scope.py / test_notes_fail_closed_auth.py all
operate on the same module object the application uses.

The canonical module does NOT depend on the shim — routes/note/note_routes.py
imports only from core/, src/, and stdlib. The outbound email cross-domain
imports (routes.email_routes._get_email_config, routes.email_helpers.
_send_smtp_message) are function-local lazy imports that keep resolving
through the email module's own path (email is not yet migrated).

One source-introspection test site repointed to the new canonical path:
- test_model_helper_owner_scope.py (shared with history; history entry
  already repointed in odysseus-dev#5090, note entry repointed here)

Adds tests/test_note_routes_shim.py to pin the sys.modules shim contract
(legacy and canonical paths resolve to the same module object; monkeypatch
via legacy alias reaches the canonical module).

Verified: compileall clean; full suite 4487 passed, 3 skipped.
@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jul 5, 2026
@ydonghao

ydonghao commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Friendly ping for review 👋

Slice 2f of the route-domain reorg, same pattern as the merged gallery (#4903), research (#4975), memory (#5007), history (#5090), and contacts (#5227) slices. Only the note domain moves — a single-file domain (915 lines, zero internal routes/ coupling at module top) — into routes/note/ behind the same sys.modules shim.

Both points from your #4903 review are baked in from the start here too:

  • the canonical module imports only from core//src//stdlib — no dependency on the legacy shim;
  • a test_note_routes_shim.py pins the shim contract (same-object + monkeypatch reach-through).

CI is fully green (pytest 4487 passed). Happy to address any feedback.

@RaresKeY @alteixeira20 — would you mind taking a look when you have a moment?

jdmanring pushed a commit to jdmanring/odysseus that referenced this pull request Jul 7, 2026
…afts

Per the search-before-staging rule, each affected draft now states
complements/conflicts vs merged and in-flight upstream work (see
upstream-recon-2026-07-07.md): tool-parsing (odysseus-dev#5033/odysseus-dev#5275/odysseus-dev#5199), provider hosts
+ native-tool toggle (odysseus-dev#4729/odysseus-dev#5206), injection header + guard benchmark (odysseus-dev#4991),
GGUF include filter (odysseus-dev#5136/odysseus-dev#5137), ntfy/note refactor (odysseus-dev#5208/odysseus-dev#5236), skills +
teacher volatility (odysseus-dev#5261/odysseus-dev#5215/odysseus-dev#4962).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@RaresKeY

Copy link
Copy Markdown
Collaborator

Quick reminder: I tried to take another look at head 40a3c7c, but the branch currently conflicts with the target branch, so I can't review it cleanly yet. Please rebase/update the PR and resolve the conflicts in app.py and routes/note_routes.py; I'll take another pass once it's clean.

This is not a code finding. Source review did not proceed because the head could not be prepared against the current target branch.

Resolves conflicts after upstream commits d16b849 (odysseus-dev#5420, harden attachment
lifecycle) and 4901a96 (odysseus-dev#5208, sanitize ntfy Title header) touched
routes/note_routes.py and app.py:

- app.py: keep canonical import path (routes.note.note_routes) + preserve
  upstream's new upload_handler kwarg on setup_note_routes().
- routes/note/note_routes.py (canonical): updated to upstream version (937
  lines, includes attachment hardening + ntfy sanitize changes).
- routes/note_routes.py (shim): kept as the 18-line sys.modules replacement.

No behavior change from the merge itself — upstream functional changes are
preserved verbatim in the canonical location; the migration structure (shim
+ subpackage) is unchanged.

Verified: compileall clean; full suite 4608 passed, 3 skipped.
@ydonghao

Copy link
Copy Markdown
Contributor Author

Quick update — this branch was behind dev and had merge conflicts. I've resolved them in 05556e8:

Upstream commits #5420 (attachment lifecycle hardening) and #5208 (ntfy Title sanitize) touched routes/note_routes.py and app.py after this PR was opened. Resolution:

  • app.py: kept the canonical import path (routes.note.note_routes) + preserved upstream's new upload_handler kwarg on setup_note_routes().
  • routes/note/note_routes.py (canonical): updated to upstream version — includes the attachment hardening + ntfy sanitize changes verbatim.
  • routes/note_routes.py (shim): unchanged — still the 18-line sys.modules replacement.

No behavior change from the merge. CI re-ran green (pytest 4608 passed, 0 failed). Ready for review when you have a moment.

@RaresKeY @alteixeira20

@ydonghao

Copy link
Copy Markdown
Contributor Author

Hi @RaresKeY — following up on your earlier note: the conflicts you flagged in app.py and routes/note_routes.py are resolved in 05556e8 (see my previous comment for details). The branch now merges cleanly into dev and CI is green (pytest 4608 passed, 0 failed). Ready whenever you're able to take that second pass you mentioned. Thanks!

@RaresKeY RaresKeY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I checked the latest head: the note implementation is preserved under the canonical package path, application registration keeps the existing setup arguments, and the legacy shim keeps old and new imports on the same module object. I did not find any author-actionable issues.

Validation

  • Visible CI checks on this head passed.
  • Focused isolated tests passed (33 tests), and the changed Python files compiled successfully.
  • I did not run the full test suite or a live application smoke test.

@RaresKeY
RaresKeY merged commit e57f60b into odysseus-dev:dev Jul 20, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants