doctor: scope agnix lint to canonical content - #117
Merged
Conversation
Reviewer's GuideScopes agnix lint diagnostics in Sequence diagram for agnix lint scoping in dotagents doctorsequenceDiagram
participant Doctor
participant agnix
participant scopeAgnixToCanonical
Doctor->>agnix: runAgnix(repoRoot)
agnix-->>Doctor: agnixReport bytes
Doctor->>Doctor: parseAgnixReport(out)
Doctor->>scopeAgnixToCanonical: scopeAgnixToCanonical(report)
scopeAgnixToCanonical-->>Doctor: agnixReport, ignored
Doctor->>Doctor: agnixDetail(report)
alt report.Summary.Errors > 0
Doctor-->>Doctor: return checkResult fail with detail
else report.Summary.Errors == 0
Doctor-->>Doctor: return checkResult pass with detail
end
Flow diagram for scopeAgnixToCanonical filtering of external diagnosticsflowchart TD
A[agnixReport input] --> B[iterate diagnostics]
B --> C{file path
starts with external/ or
contains /external/}
C -->|yes| D[ignored++]
D --> B
C -->|no| E[append to kept]
E --> F{diagnostic level}
F -->|error| G[summary.Errors++]
F -->|warning| H[summary.Warnings++]
F -->|other| I[summary.Info++]
G --> B
H --> B
I --> B
B --> J[replace report.Diagnostics with kept]
J --> K[set report.Summary from summary]
K --> L[return report, ignored]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
checkAgnix, the error fromrunAgnixis now ignored, which means genuine failures (e.g., agnix crashing) are treated the same as a clean run; consider preserving or surfacing that error separately from lint findings. - The
scopeAgnixToCanonicalpath check only matchesexternal/and/external/, so files under paths like./external/...or other variations might slip through; consider normalizing and stripping leading./or using a more robust path prefix check.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `checkAgnix`, the error from `runAgnix` is now ignored, which means genuine failures (e.g., agnix crashing) are treated the same as a clean run; consider preserving or surfacing that error separately from lint findings.
- The `scopeAgnixToCanonical` path check only matches `external/` and `/external/`, so files under paths like `./external/...` or other variations might slip through; consider normalizing and stripping leading `./` or using a more robust path prefix check.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: d50a4b9716
ℹ️ 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".
Comment on lines
+342
to
+344
| if strings.HasPrefix(normalized, "external/") || strings.Contains(normalized, "/external/") { | ||
| ignored++ | ||
| continue |
There was a problem hiding this comment.
When a user still has a non-materialized external skill, which this repo keeps supported via TestLegacyDirectExternalSourceRemainsDeliverableWithoutMaterialization, and the config root is the documented in-place ~/.agents checkout, that delivered skill lives under repoRoot/external/...; this filter drops every agnix diagnostic for it even though expectedSkills still includes non-materialized sources and sync symlinks them into harness skill dirs. The separate external audit only checks risky regex patterns, not agnix’s syntax/frontmatter rules (upstream JSON mode exits nonzero when summary.errors > 0), so malformed direct external skills can now make doctor pass instead of failing.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Residual from the v0.3.1 funnel retest: fresh-install
doctorstill exited 1 because agnix linted the fetchedexternal/upstream clone (mattpocock/skills) alongside canonical content.doctornow drops agnix findings underexternal/and recomputes the summary from the kept diagnostics, noting how many upstream findings were ignored. External sources keep their own dedicated audit check. With this, a brand-new user's firstdotagents doctoris green.Summary by Sourcery
Scope agnix lint results in
dotagents doctorto canonical repository content and ignore issues from external upstream clones.Bug Fixes:
dotagents doctorruns from failing due to agnix findings in fetchedexternal/upstream clones rather than local content.Enhancements:
external/and recompute summary counts based only on canonical content, while annotating how many external findings were ignored in the doctor output.Tests:
external/clones are ignored and summary counts are recomputed from remaining findings.