Reference STYLE.md instead of importing it, and fix two wrong claims - #587
Conversation
@STYLE.md sat on line 1, so 2,369 bytes of Go conventions loaded into every session regardless of what it was about -- a third of the always-on budget for guidance most sessions never need. It is now a pointer. Because Codex reads AGENTS.md without expanding imports, that cost fell on Claude Code alone. Two corrections: - The completeness bar listed "Registration in root.go" directly under "Command file in internal/commands/", which reads as though root.go were in that directory. It is internal/cli/root.go. - Skill drift was described as checking skills/basecamp/SKILL.md. The Makefile target runs the script twice -- once on that default and once on skills/basecamp-doctor/SKILL.md -- so changing a command could leave the second skill stale while the doc said nothing about it. Always-on drops from 7,285 to 5,150 bytes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d42c16a1d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Updates the agent-facing development guidance to reduce always-on context by removing the inline STYLE.md import from AGENTS.md, while correcting two documentation inaccuracies so the guidance matches the repo’s actual paths and checks.
Changes:
- Removed the
@STYLE.mdimport and replaced it with a lightweight reference toSTYLE.md. - Corrected the skill drift guidance to reflect that
make check-skill-driftvalidates bothskills/basecamp/SKILL.mdandskills/basecamp-doctor/SKILL.md. - Corrected the “completeness bar” registration path to
internal/cli/root.go.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Saying "update both skills to match" was too strong. check-skill-drift.sh verifies that commands and flags a skill references still exist in .surface, so adding a command breaks neither skill, while renaming or removing one breaks whichever skill mentioned it. basecamp-doctor intentionally covers only doctor, setup and auth remediation, and should not be edited for an unrelated command. The Makefile does run the check over both files, which is what the previous wording was reaching for; that part is kept.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc7fe464d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Saying removal breaks whichever skill mentioned the command was too strong. resolve_cmd walks up to the nearest existing ancestor, so a reference to a removed nested subcommand resolves against its parent and the check exits clean. Probed it: adding 'basecamp setup definitely-removed-subcommand' to basecamp-doctor's skill passes the current checker. Top-level removal is caught; subcommand removal is not. Documented that way so nobody relies on CI to catch it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
AGENTS.md:138
- This section references STYLE.md in code formatting, but elsewhere (e.g., CONTRIBUTING.md) the doc uses clickable links. Using a markdown link keeps the intent (don’t import) while making navigation easier.
See `STYLE.md` for the Go conventions used here. Read it when writing or reviewing Go;
it is not imported, so it stays out of context for sessions that never touch Go.
AGENTS.md:71
- The paragraph about subcommand removals has an overlong line and uses a semicolon to join sentences, which is inconsistent with the surrounding wrapped prose and makes the guidance harder to scan.
This issue also appears on line 137 of the same file.
check still passes — `basecamp setup <removed>` resolves as `basecamp setup`. Removing a
top-level command is caught; removing a subcommand is not. Don't lean on CI for this. Update the skill the change actually affects;
basecamp-doctor deliberately covers only doctor, setup and auth remediation.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
@STYLE.md sat on line 1, so 2,369 bytes of Go conventions loaded into every
session regardless of what it was about -- a third of the always-on budget for
guidance most sessions never need. It is now a pointer. Because Codex reads
AGENTS.md without expanding imports, that cost fell on Claude Code alone.
Two corrections:
"Command file in internal/commands/", which reads as though root.go were in
that directory. It is internal/cli/root.go.
target runs the script twice -- once on that default and once on
skills/basecamp-doctor/SKILL.md -- so changing a command could leave the
second skill stale while the doc said nothing about it.
Always-on drops from 7,285 to 5,150 bytes.
Checked against
mainat c863c3d.Summary by cubic
Replaced the inline import of
STYLE.mdinAGENTS.mdwith a link, reducing always-on context by ~2.3 KB. Also fixed the completeness path and clarified that skill-drift checks catch stale references and do not catch subcommand removals.Refactors
STYLE.mdinstead of importing it inAGENTS.md(always-on drops from 7,285 to 5,150 bytes).Bug Fixes
internal/cli/root.go(pluscommands_test.go).make check-skill-driftruns over both skills and flags stale references, not missing coverage; top-level removals are caught, subcommand removals are not (CI won’t catch them).Written for commit df317c8. Summary will update on new commits.