Skip to content

PR - issue #20 Add negative-path REST tests for note retrieval and import validation#27

Open
finnrea78 wants to merge 5 commits intoGenentech:mainfrom
finnrea78:main
Open

PR - issue #20 Add negative-path REST tests for note retrieval and import validation#27
finnrea78 wants to merge 5 commits intoGenentech:mainfrom
finnrea78:main

Conversation

@finnrea78
Copy link

⏺ Acceptance Criteria Coverage

Completed:

  • GET /v1/notes/{note_id} returns 404 with "Note not found." for a missing note.
  • PATCH /v1/notes/{note_id} returns 404 with "Note not found." for a missing note.
  • POST /v1/notes/import returns 400 with "Only .txt files are supported." when a non-.txt file is uploaded.
  • POST /v1/notes/import returns 400 with "Unsupported text encoding. Use UTF-8, UTF-8 BOM, or UTF-16." for an unsupported text encoding.
  • Each new test asserts both the HTTP status code and the response detail message.
  • Existing happy-path note tests still pass.

Not completed:

  • None — all acceptance criteria were completed.

Validation

PYTHONPATH=src python -m unittest discover -s src/move37/tests -t src

  • Python tests: 26 ran, 24 passed, 1 pre-existing failure (test_fork_clears_scheduling_fields — NameError: name 'remove_ids' is not defined in activity_graph.py:380), unrelated to this issue.
  • All 4 new tests pass:
    • test_get_note_returns_404_for_missing_note
    • test_patch_note_returns_404_for_missing_note
    • test_import_rejects_non_txt_file
    • test_import_rejects_unsupported_encoding

Prompt History

  • Prompt 0: Explore the Move37 repository — understand the project structure, architecture, tech stack, and what it does.
  • Prompt 1: List all open GitHub issues and assess their difficulty and suitability for someone without a coding agent.
  • Prompt 2: Read the relevant files for issue Add negative-path REST tests for note retrieval and import validation #20 — notes.py (service), notes.py (router), and test_api.py — to understand the error mapping and existing test patterns.
  • Prompt 3: What errors does the note service raise, and how does the router map them to HTTP responses?
  • Prompt 4: Implement test for GET missing note returning 404.
  • Prompt 5: Implement tests for import with non-.txt file and unsupported encoding.
  • Prompt 6: Debug test discovery failure — ImportError: Start directory is not importable. Found missing init.py in tests directory.
  • Prompt 7: Debug Python version issue — system Python was 2.7, needed 3.12+. Used uv to install and create venv.
  • Prompt 8: Debug encoding test failure — find byte sequences that fail all three decoders (utf-8-sig, utf-8, utf-16).

AI Mistakes And Corrections

Issue: The assistant suggested b'\x80\x81\x82\x83' as bytes that would fail all three text decoders (utf-8-sig, utf-8, utf-16).
Correction: These bytes are 4 bytes (even length), which allows successful decoding as UTF-16. Changed to b'\x80' (single byte, odd length) which genuinely fails all three decoders.

Issue: The test file had a broken stub (def test_ with no body) introduced during editing, causing a SyntaxError that prevented the entire test module from loading.
Correction: Removed the incomplete stub to restore test discovery.

Observations And Repo Feedback

Issues encountered during onboarding that are not part of the issue scope but worth noting:

  • README does not show how to run the project — setup steps are buried in contributing-docs/docs/local-development.md.
  • Broken local file path in README line 33 (/Users/pereid22/source/...).
  • No .env.example provided.
  • Missing init.py in src/move37/tests/ — documented test command fails without it.
  • Pre-existing test failure in test_fork_clears_scheduling_fields (remove_ids undefined).
  • No pyproject.toml for the main app — multiple scattered python-requirements.txt files make setup fragile. uv with a single pyproject.toml would simplify onboarding significantly.
  • docker compose up -d --build db api web pulls 7 containers due to dependency chain, but most hiring issues only need local unit tests.
  • Podman compose doesn't handle the depends_on build ordering correctly — Docker-only assumption not documented.

Candidate Checklist

  • I explained which issue acceptance criteria were completed, partially completed, or not completed.
  • I validated the change with tests and/or manual checks.
  • I included the key prompts and exploratory questions that materially influenced the work.
  • I documented any important assistant mistakes and how I corrected them.
  • If I changed direction, I explained why. (N/A — stayed on issue Add negative-path REST tests for note retrieval and import validation #20)
  • I documented any limitations, deferred work, edge cases, or improvements I noticed but did not address.

TODO notes (Finn):

Onboarding Friction

README gaps:

  • No quick-start section — candidates have to navigate to contributing-docs/docs/local-development.md to learn how to run anything.
  • No clear description of what the project is trying to achieve. "Human operating system for the AI age" needs unpacking — what does a candidate actually interact with?
  • Broken link on line 33 — points to /Users/pereid22/source/penrose-lamarck/.github/pull_request_template.md (local path on someone else's machine).
  • PR template is referenced but not findable from the README.
  • No link to the contributing docs as a rendered site.

Python environment is fragile:

  • No pyproject.toml for the main app — instead there are 4 separate python-requirements.txt files across api/, db/, alembic/, and devtools/. Easy to miss one.
  • No .env.example provided.
  • No virtual environment guidance — the documented pip install command installs into whatever Python is active.
  • The documented test command fails out of the box because src/move37/tests/init.py is missing from the repo.
  • Recommend using uv with a single pyproject.toml — would reduce setup to uv sync && uv run python -m unittest ....

Docker Compose is heavyweight for the task:

  • The "fastest" startup path (db api web) silently pulls 7 containers (including Milvus, etcd, MinIO) due to the dependency chain. Most hiring issues are unit tests that don't need Docker at all.
  • Podman Compose doesn't handle the depends_on + build ordering — move37-ai fails to build, which blocks api and web. Docker-only assumption isn't documented.

OpenAI API key:

  • Semantic search and chat features require OPENAI_API_KEY. Not clear whether candidates are expected to provide their own key or if this is provided. Could be a blocker.

Pre-existing Bugs

  • test_fork_clears_scheduling_fields fails with NameError: name 'remove_ids' is not defined in activity_graph.py:380. This is a bug in the source, not the test.
  • Extensive SQLAlchemy SAWarning spam during test runs (identity map conflicts with SQLite) — harmless but noisy and potentially alarming for candidates.

What Works Well

  • Issue descriptions are excellent — clear acceptance criteria, file pointers, non-goals, and validation commands.
  • Good mix of issue types across Python tests, SDK tests, and frontend work.
  • Architecture documentation in contributing-docs/ is thorough and well-structured once you find it.
  • Clean layered code (routes, services, repositories, models) — easy to follow once oriented.
  • Test patterns are consistent and easy to replicate for new tests.
  • The exercise design of asking candidates to document their prompts and AI corrections is a smart way to evaluate agent-assisted workflow.

Added a test section to the README.
Removed the placeholder 'test' line from the README.
Update test to use a single byte for unsupported encoding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant