feat: support remote ALCops configuration - #500
Conversation
|
@someC0d3r, I was planning to review your PR, but I see you've closed it. Is this by accident or do you want to create a new PR? |
|
@Arthurvdv I'm gonna reopen it then, then you can review it! :) |
There was a problem hiding this comment.
Thanks for this — the proof-of-concept is solid and this work shouldn't be wasted. The resolver core matches what was agreed in #483: merge semantics, single-level inheritance, separate validation of the inherited document, and the Lazy cache fix are all keepers. After reviewing this against the NAV SDK's external-ruleset handling, we've settled on requirements that change the failure behavior, so the plan is to sequence this rather than polish it now:
- Sequencing: configuration-load diagnostics (#328) must land first — a failure to load
alcops.jsonor anExtendssource must surface as a diagnostic, never silently. I'll create a PR for #328 myself; once that's merged, this PR can be rebased onto it and build on that mechanism. - All-or-nothing fallback: when
Extendsis declared and cannot be fully resolved (unreachable, timeout, malformed, or the source itself declaresExtends), the effective configuration falls back to built-in defaults entirely — local overrides are ignored too, to avoid a confusing half-applied state. The diagnostic from #328 makes the drop visible. - Hardening: add a response size cap on the HTTP fetch (e.g. 1 MB). No URL gating beyond the existing credentials-in-URL rejection; we'll document that committing
alcops.jsonimplies trusting the referenced source. - Everything else stays as designed here: single source, HTTP(S) or absolute local path, no inheritance chains, once-per-process caching, local-over-remote merge with array replacement.
So concretely: I'll keep this PR open, get #328 in first, and ping you here when it's merged — then a rebase plus the failure-behavior changes (points 2 and 3, with the silent-fallback tests updated to assert the diagnostic + defaults behavior) should get this over the line.
This review was created with Claude.
|
@someC0d3r I've implemented the warning mechanism when we can't load the ALCops settings, where I the idea was make it extendable for the Extends.Source feature. If you take the changes from the main branch ideally you could build this on top of the improvements from the #512. Ping me if i can assist or help on anything. Would be great to bring this into the next version of ALCops 🤗 |
|
No worries! I'll merge the behinds and give you feedback here when I'm done. :) |
…tends # Conflicts: # .claude/rules/common-library.md # src/ALCops.Common/Settings/ALCopsSettingsProvider.cs
|
Following the model switch from GPT-5.6 Sol to GPT-6 Astra (both using Very High reasoning), GPT-6 Astra reviewed this PR and merge commit Decision basis for findings 1 and 2: I used the explicit requirements published by Arthurvdv in his maintainer review of 4 September 2026. For the fallback, point 2 states:
The review treats the declared base and local overrides as one configuration and asks for complete built-in defaults if inheritance fails, to avoid applying only part of that configuration. Continuing with valid local settings was the original design and is technically possible. I classified it as a finding because it differed from this newly requested policy; CM0001 itself does not require discarding local settings. For HTTP hardening, point 3 requests:
The review suggests approximately 1 MB. The credential disclosure in finding 3 was an additional issue identified during this Codex review. The linked maintainer review itself discloses that it was created with Claude; I treated the requirements published under Arthurvdv's account as the maintainer's instructions. The CM0001 foundation from #512 is present: the Common-hosted analyzer, diagnostic metadata, compilation-level reporting, cached failures, and original tests were preserved. However, three issues remain:
All 116 existing Common tests passed on each of .NET 10, .NET 8, and the legacy AL 12 setup, which confirms that these specific gaps need additional regression coverage. I am addressing these findings with failing regression tests first, followed by the implementation and framework checks. The configuration guide and companion documentation PR ALCops/alcops.dev#161 will be updated to describe the resulting behavior. A follow-up comment will report the changes and actual validation results once the fixes are pushed. Generated via Codex using GPT-6 Astra with Very High reasoning. |
|
The three findings from the review are fixed and pushed in 35e96a8. Decision basis for changes 1 and 2: These implement the explicit requirements published by Arthurvdv in his maintainer review of 4 September 2026. For the fallback, point 2 states:
Its stated rationale is to avoid applying only part of the intended base-plus-overrides configuration. I therefore changed the original local-fallback behavior to complete built-in defaults. Using valid local settings would also be technically possible; this change implements the requested policy rather than a technical requirement imposed by CM0001. For HTTP hardening, point 3 requests:
The review suggests approximately 1 MB; the concrete limit selected in this implementation is 1 MiB (1,048,576 bytes). Credential redaction in change 3 addresses an additional issue identified during this Codex review. The linked maintainer review itself discloses that it was created with Claude; I treated the requirements published under Arthurvdv's account as the maintainer's instructions.
TDD and validation: the regression run against the previous implementation produced 23 expected failures and 110 passes before production code changed. After the fix, all 133 Common tests pass on each of AL 18, AL 16, and AL 12. Coverage includes HTTP size boundaries, UTF-8 byte counting, HTTP errors/body timeouts, invalid bases and source declarations, and actual CM0001 messages. All seven cop suites were then run:
All seven production projects built for all three supported TFMs. Formatting verification and validation of all 52 internal rule guides passed. Existing NU1900/MSB3277 warnings remain in the local test logs; these results are local validation, not GitHub CI results. The integration of #512 through merge The README, schema and internal guidance are aligned with the fixes. Companion docs PR ALCops/alcops.dev#161 now updates both the configuration guide and CM0001 page and builds successfully with Hugo Extended (167 pages). Both PR descriptions have been refreshed to match the final behavior. No remaining blocker was found in the reviewed changes. This PR remains a Draft. Generated via Codex using GPT-6 Astra with Very High reasoning. |
|
@Arthurvdv Codex performed the merge and I reviewed the changes afterwards. Since GPT-6 Astra was rolled out, I switched models and had it perform an AI review of this PR. It identified a few additional changes and created a fix commit to address the points you mentioned FYI Also the docs PR got updated: ALCops/alcops.dev#161 |
Summary
Adds
Extends.Sourceso projects can share one centrally maintainedalcops.jsonand override selected settings locally, as proposed in #483.Integrates the central CM0001 diagnostic from #512 and addresses the maintainer review: failed inheritance uses complete defaults, HTTP responses have a size limit, and rejected URL credentials are removed from diagnostics.
Behavior
Content-Length. The five-second timeout also covers the response body.41becomes the default8if its base cannot be loaded.The JSON schema, README, internal guidance, configuration tests and CM0001 tests describe and verify the same behavior. Companion documentation: ALCops/alcops.dev#161.
Local validation
Regression tests were changed/added before the production fix: 23 failed, 110 passed against the previous implementation. After the fix, all 133 Common tests pass in each SDK configuration.
netstandard2.1,net8.0, andnet10.0before the CI-mode tests.dotnet format --verify-no-changesand.claude/scripts/Validate-Rules.ps1passed; the latter checked 52 rule files.Codex disclosure
The initial implementation was generated using Codex. Original author note: 5.6 Sol, Reasoning: Very High (4/5).
The follow-up review and these corrections were performed using Codex with GPT-6 Astra, Reasoning: Very High.
Generated via Codex using GPT-6 Astra with Very High reasoning.