Skip to content

Refresh architecture docs and memory map#61

Merged
wwind123 merged 2 commits into
mainfrom
docs-refresh-architecture-diagram
May 2, 2026
Merged

Refresh architecture docs and memory map#61
wwind123 merged 2 commits into
mainfrom
docs-refresh-architecture-diagram

Conversation

@wwind123
Copy link
Copy Markdown
Owner

@wwind123 wwind123 commented May 2, 2026

Summary

  • update the architecture diagrams to show memory preparation, subprocess execution, follow-up handling, optional tests, logs, and GitHub operations
  • make agent-memory wording consistently repo-scoped
  • expand generated architecture-map memory with module and supporting-surface sections

Tests

  • python -m pytest

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Implemented and opened PR #61: #61

Updated the architecture diagrams, made agent-memory wording consistently repo-scoped, and expanded the generated cached architecture map with module/supporting-surface sections plus test coverage.

Verification: python -m pytest passed, 127 tests.

-- OpenAI Codex

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Now I have all the information needed to write a complete review.


PR #61 Review: Refresh architecture docs and memory map

Summary

This PR makes three categories of changes:

  1. Documentation updates to docs/local_agent_loop.md (flowchart + sequence diagram)
  2. A wording consistency fix (repo-localrepo-scoped) in cli.py and memory.py
  3. Expanded _write_architecture_map output in memory.py + matching test assertions

Code correctness

memory.py_write_architecture_map: The new sectioning logic is correct. For the default FakeRunner fixture (tracked_files includes src/coding_review_agent_loop/cli.py, README.md, tests/test_agent_loop.py, pyproject.toml):

  • Path bucketing: src/coding_review_agent_loop/cli.py has 3 parts → falls to the else branch → module key becomes src/coding_review_agent_loop. Files with 4 parts (e.g. src/pkg/agents/claude.py) take the if len(parts) >= 4 branch → src/pkg/agents. Both cases are correct.
  • Edge cases (no Python files, no docs/tests/workflows) are guarded with if python_files: and if docs or tests or workflows:.

cli.py wording fix: Correct and necessary. The --no-agent-memory help text was the only remaining instance of repo-local; every other occurrence in cli.py already used repo-scoped (lines 70, 76, 82, 189, 207).

Test assertions: The four new assertions are well-chosen and verify real structural properties of the generated content. They all pass given the default FakeRunner.tracked_files.

Documentation accuracy

The sequence diagram additions are accurate and consistent with the actual code: orchestrator.py calls prepare_agent_memory (lines 228, 286, 363) and ensure_agent_workdirs (lines 225, 285, 360), matching Orch->>Orch: ensure active agent workdirs and Orch->>Memory: prepare advisory repo memory.

Same-PR follow-ups

Flowchart inconsistency in docs/local_agent_loop.md: The flowchart places memory preparation under Config (Config --> Memory[Agent memory preparation<br/>memory.py]), but the sequence diagram in the same file — and the actual code in orchestrator.py — show it is the Orchestrator, not Config, that initiates memory preparation. The Config node in the flowchart should be a child of Orchestrator, not a sibling peer:

-    Config --> Memory[Agent memory preparation<br/>memory.py]
-    Config --> Logs[(Agent logs<br/>.agent-loop-logs)]
+    Orchestrator --> Memory[Agent memory preparation<br/>memory.py]

and the Runner --> Logs edge (added by this PR) can handle the logs connection. This aligns the two diagrams and removes the factual mismatch.


-- Anthropic Claude

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

The changes in this PR significantly improve the structural context provided to agents via the architecture map and update the documentation to match the current capabilities of the orchestrator.

Summary

  1. Architecture Map Expansion: src/coding_review_agent_loop/memory.py now generates a more detailed architecture-map.md with sections for top-level layout, Python modules (grouped by package), and supporting surfaces (docs, tests, workflows). This provides better orientation for agents.
  2. Consistency: Standardized on "repo-scoped" instead of "repo-local" for agent memory in cli.py and memory.py.
  3. Documentation: Updated docs/local_agent_loop.md with a more comprehensive Mermaid flowchart and an updated sequence diagram that includes memory preparation and follow-up handling.
  4. Tests: Added assertions in tests/test_agent_loop.py to verify the new sections in the architecture map.

Code Quality and Correctness

  • The path bucketing logic in _write_architecture_map correctly handles both shallow and deep Python module structures.
  • The wording changes are consistent and avoid confusion between the global cache (repo-scoped) and local repository files.
  • Tests pass successfully, and the new assertions verify the structural integrity of the generated memory.

Same-PR follow-ups

  • Flowchart Inconsistency: In docs/local_agent_loop.md, the Mermaid flowchart shows the Config node leading to Memory and Logs. However, as correctly shown in the sequence diagram and implemented in orchestrator.py, it is the Orchestrator that initiates memory preparation and utilizes the Runner for logging. The Memory and Logs nodes (and potentially Workdirs) should be children of Orchestrator to accurately reflect the control flow.

-- Google Gemini

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Implemented and pushed the same-PR fix.

Changed docs/local_agent_loop.md so the flowchart now shows config feeding the orchestrator, with the orchestrator initiating workdir setup and memory preparation. Removed the stale Config --> Logs edge; logging remains represented through Runner --> Logs.

Verification: python -m pytest passed, 127 tests.

Pushed commit: cb9a981 Fix architecture flowchart control flow

-- OpenAI Codex

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

The round 1 blocking issue has been fully resolved. The flowchart now correctly shows Config --> Orchestrator and Orchestrator --> Workdirs/Memory, which is consistent with the sequence diagram and the actual call chain in orchestrator.py. The cli.py wording fix, memory.py expansion, and test assertions are unchanged and were already correct.

The Logs node loses its label/shape (the old [(Agent logs<br/>.agent-loop-logs)] definition was removed along with Config --> Logs, leaving only the unlabeled Runner --> Logs reference), but this is a cosmetic rendering difference and not worth another blocking round.

-- Anthropic Claude

@wwind123
Copy link
Copy Markdown
Owner Author

wwind123 commented May 2, 2026

Warning: True color (24-bit) support not detected. Using a terminal with true color enabled will result in a better visual experience.
YOLO mode is enabled. All tool calls will be automatically approved.
YOLO mode is enabled. All tool calls will be automatically approved.
I will start by fetching the PR details and the diff to understand the changes.
I will run the tests to confirm that everything is passing and that the new assertions are valid.
I will read the test_agent_memory_is_created_and_added_to_review_prompt test in tests/test_agent_loop.py to confirm the tracked_files being used and verify the assertions.
I will search for the definition of FakeRunner to find the default tracked_files.
I will read the entire docs/local_agent_loop.md file to see if the Logs node is defined elsewhere and to check the overall structure of the Mermaid diagrams.
I will check the current git branch and the last few commits to verify if I am on the correct branch and to see the most recent changes.
Error executing tool run_shell_command: Tool execution for "Shell" requires user confirmation, which is not supported in non-interactive mode.
I will re-run gh pr diff to confirm the exact changes in the PR and ensure I have the most up-to-date information.
I will use grep to search for specific lines in docs/local_agent_loop.md to determine the current state of the file and identify any inconsistencies.
I will read .git/HEAD to determine the current commit SHA and verify if I am on the correct branch.
Error executing tool read_file: File path '/tmp/coding-review-agent-loop/wwind123-coding-review-agent-loop/gemini/repo/.git/HEAD' is ignored by configured ignore patterns.
I will use gh to view the changes in commit cb9a981 to see if it contains the correct modifications and to understand why the workspace might be in an inconsistent state.
The changes in this PR significantly improve the architecture documentation and the richness of the structural context provided to agents through the advisory memory.

Summary

  1. Architecture Docs Refresh: Updated docs/local_agent_loop.md with a corrected flowchart and an expanded sequence diagram. The flowchart now accurately reflects the control flow where Config validates settings and then the Orchestrator manages workdirs and memory.
  2. Enhanced Architecture Map: The architecture-map.md generated by memory.py now includes dedicated sections for "Top-level Layout", "Python Modules" (grouped by package), and "Supporting Surfaces" (docs, tests, workflows).
  3. Terminology Alignment: Standardized on "repo-scoped" instead of "repo-local" for agent memory to better describe its lifecycle and scope.
  4. Test Coverage: Added assertions to ensure the new architecture map sections are correctly generated.

Code Quality and Correctness

  • The module bucketing logic in _write_architecture_map is well-implemented, handling both top-level source files and nested packages appropriately.
  • The documentation updates are consistent with the implementation in orchestrator.py and memory.py.
  • All tests passed, verifying that the changes do not introduce regressions in the memory preparation logic.

The round 1 blocking issue regarding the flowchart inconsistency has been fully addressed in commit cb9a981. While the Logs node in the flowchart now uses a default box shape instead of the previous cylinder, this is a minor cosmetic change and does not affect the clarity of the diagram.

-- Google Gemini

@wwind123 wwind123 merged commit 2029085 into main May 2, 2026
1 check passed
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