Add AGENTS.md, code review guidelines, and architecture docs#9847
Add AGENTS.md, code review guidelines, and architecture docs#9847MostCromulent wants to merge 3 commits intoCard-Forge:masterfrom
Conversation
- AGENTS.md: tool-agnostic AI coding agent configuration with project overview, build commands, and development principles - docs/Development/Guidelines.md: code review guidelines distilled from PR feedback (general principles, code style, architecture, network patterns, testing) - docs/Development/Architecture.md: GUI inheritance hierarchy, layer responsibilities, and network infrastructure reference - CONTRIBUTING.md: new sections pointing to guidelines and AGENTS.md - docs/_sidebar.md: new entries under Development Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Seeing some weird takes on Agents.md right now https://umesh-malik.com/blog/agents-md-ai-coding-agents-study |
|
TBH, I think the AGENTS.md is far less important than the guidelines and architecture docs. It's really just a means to an end in trying to make sure the AI takes them into account when planning and coding. i.e. with reference to the article above:
This is basically what the guidelines are: starting from a blank slate, here's everything the AI screwed up that has been identified in PR feedback, and the instructions to avoid it happening again. (Though I accept there's an argument that extracting this to a guidelines doc is really just an AGENTS.md with more steps). |
Jetz72
left a comment
Jetz72
left a comment
There was a problem hiding this comment.
Not opposed to introducing some documentation along these lines, but I think it would need a lot of expansion and refinement by people versed in Forge's systems, and the involvement of other experienced contributors.
docs/Development/Guidelines.md
Outdated
| - **Keep engine clean:** GUI-specific logic (UI hints, styling) belongs in View classes, not in forge-game engine classes like Player.java or PhaseHandler.java. | ||
| - **Fix bugs at the closest layer:** Errors and bug fixes should be solved in the closest layer that is effective. For example, a network serialization issue should be fixed in the network layer, not by adding guards in the game engine. A card rules bug belongs in forge-game, not worked around in forge-gui. Fixing at the source keeps the codebase clean and avoids defensive code proliferating through unrelated layers. | ||
| - **Platform-neutral code for platform-neutral features:** If a feature is intended to work across platforms (desktop and mobile), implement the *state and logic* in shared code (e.g., `AbstractGuiGame`, `forge-gui`) rather than in platform-specific classes. Display that uses platform-specific APIs (Swing components, libgdx widgets) belongs in platform subclasses (`CMatchUI`, `MatchController`). However, simple text messages and lightweight UI logic that is identical across platforms may live in `AbstractGuiGame` to avoid duplication — prefer one implementation in the base class over two identical copies in subclasses. **Code smell:** If you find yourself writing the same algorithm with the same state fields in both `CMatchUI` and `MatchController`, that's a strong signal it belongs in `AbstractGuiGame` instead. | ||
| - **Check for mobile GUI:** Desktop-only features must check `GuiBase.getInterface().isLibgdxPort()` and return early/disable for mobile. Users switching between desktop and mobile share preferences. |
There was a problem hiding this comment.
This bit about isLibgdxPort directly contradicts the guideline in the "Red Flags" section of the architecture document.
docs/Development/Guidelines.md
Outdated
| - **Zone transitions involve two players, not one:** When a card changes zones, the "from" zone and "to" zone may belong to different players (e.g., a stolen creature dying moves from the controller's battlefield to the owner's graveyard). Don't assume the card's owner or current controller is the correct player for both zones — use each zone's actual player reference. | ||
| - **Reuse `TrackableProperty` constants across view types:** Each `TrackableObject` instance has its own property map, so different view classes (e.g., `SpellAbilityView` and `StackItemView`) can share the same `TrackableProperty` enum constant when the semantics and type match. Don't create prefixed duplicates (e.g., `SA_Foo`) when an unprefixed constant (`Foo`) already exists with the same `TrackableType`. The enum name is just a key — it doesn't imply ownership by a particular view class. | ||
| - **Isolate network code:** Network-specific functionality should be in dedicated classes (`NetGuiGame`, `NetGameController`) rather than added to core classes like `AbstractGuiGame`. Keep core game classes free of network dependencies so they remain usable in non-network contexts. | ||
| - **Use `GuiBase.isNetworkplay(game)` for network detection:** There is only one signature — `isNetworkplay(IGuiGame game)`. When a game reference is available, pass it; the method delegates to `game.isNetGame()` for a per-instance answer. When no game is available, pass `null`; the method falls back to `IGuiBase.hasNetGame()` which iterates registered game instances. **Important:** `isNetGame()` must return `true` for *all* game instances in a network match — both the server-side proxy (`NetGuiGame`) and the host's local GUI (`CMatchUI`/`MatchController`). The host's local GUI gets its flag set via `FServerManager.getGui()` calling `setNetGame()`. If adding a new code path gated on `isNetGame()`, test it from the host's perspective, not just the remote client's. |
There was a problem hiding this comment.
These four seem much more specific to specific systems than the preceding architectural guidelines. As it stands, the major points of game logic functions and the relationship between TrackableProperty/View classes to their underlying game objects are probably worthy of entire sections of notes.
Add missing items (check platform counterparts, null-guard contracts, only write meaningful tests, domain-specific section). Reduce verbosity throughout. Remove completed game event record design guideline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clarify Architecture.md covers in-match GUI only - Describe mobile UI as a full LibGDX implementation with feature parity - Reconcile isLibgdxPort guideline with Architecture red flags - Move hotkey conflicts to end of Code Style, add default prefs check - Add AI disclosure requirement to CONTRIBUTING.md - Remove multi-process test isolation and manual network testing bullets Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Have made some adjustments based on feedback so far, and to remove some of the unnecessary exposition to shorten bullet points in the guidelines. Happy to do whatever I can to help refine this, (i.e. go and make targeted changes based on feedback) but obviously it's going to rely on those with actual technical expertise, not dilettantes like me. |
3 participants
Per @tool4ever suggestion on the Discord:
Adds an AGENTS.md and supporting documentation files to Forge to help with contributions using AI coding agents.
I was going to hold off submitting this until all of my network-related PRs were done, but I've noticed more PRs being submitted by other contributors which seem to use coding agents so though it may be useful to submit this on an interim basis; it can always be updated later.
3 documents:
I suspect the Guidelines and Architecture documents are useful to both humans and AIs, so I have kept them as reference material rather than being isolated to AGENTS.md.
All of these are adapted from my current CLAUDE.md and reference documents (stripping and substituting references to stuff in development on my Fork specifically). I don't claim any of these are perfect, they may well contain things that are wrong, but they're what I'm currently using and they're a starting point.
I will say from my experience: forcing the AI to refer to the guidelines and architecture documents has been the key to avoiding a lot of the problems I had when I first started contributing. In my workflow these are automatically updated based on PR feedback, so they are getting more accurate as they are iterated - if this PR ends up being merged I'll come back to update these once all of my current network/feature PRs are finalised so it includes any relevant changes.
Of course I welcome any changes or feedback from those more learned than myself.
🤖 Generated with Claude Code