Skip to content

feat: support remote ALCops configuration - #500

Draft
someC0d3r wants to merge 4 commits into
ALCops:mainfrom
someC0d3r:feat/configuration-extends
Draft

feat: support remote ALCops configuration#500
someC0d3r wants to merge 4 commits into
ALCops:mainfrom
someC0d3r:feat/configuration-extends

Conversation

@someC0d3r

@someC0d3r someC0d3r commented Sep 1, 2026

Copy link
Copy Markdown

Summary

Adds Extends.Source so projects can share one centrally maintained alcops.json and 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

  • Loads one anonymously accessible HTTP(S) URL or one absolute local file path. The project must trust its referenced source.
  • Local scalar values and arrays replace inherited values; nested objects merge property by property. Inheritance chains are rejected.
  • HTTP responses are limited to 1 MiB (1,048,576 bytes) during buffering, including chunked responses and responses without Content-Length. The five-second timeout also covers the response body.
  • HTTP(S) URLs containing username/password information are rejected before network access. That information is omitted from the CM0001 diagnostic source.
  • If declared inheritance fails, both the base and local overrides are discarded and built-in defaults apply, with CM0001 explaining the failure. For example, a local complexity threshold of 41 becomes the default 8 if its base cannot be loaded.
  • Unknown top-level setting names remain non-fatal: recognized values apply and CM0001 identifies each unknown name. Invalid inherited values cannot be hidden by local overrides.
  • Effective settings and failures are cached per workspace path for the analyzer session. Restart the analyzer process after correcting a cached failure.

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.

SDK / analyzer target Passed Skipped Failed
AL 18.0.36.33307 / net10.0 1,739 2 0
AL 16.0.27.57058 / net8.0 1,723 18 0
AL 12.0.13.24028 / netstandard2.1, tests hosted on .NET 10 1,502 239 0
  • All seven production projects built for netstandard2.1, net8.0, and net10.0 before the CI-mode tests.
  • Regression coverage includes complete fallback, malformed/invalid bases, inheritance chains, invalid source declarations, credential redaction and rejection before connecting, exact HTTP byte-limit boundaries, chunked/no-length responses, UTF-8 byte counting, HTTP errors, body timeout, and actual CM0001 output.
  • dotnet format --verify-no-changes and .claude/scripts/Validate-Rules.ps1 passed; the latter checked 52 rule files.
  • Test logs retain existing NU1900 vulnerability-feed and MSB3277 assembly-reference warnings. The table reports local execution; Draft PRs currently skip GitHub build/test jobs.
  • Companion docs built with Hugo Extended: 167 pages, no errors; existing Hugo/Docsy deprecation warnings remain.

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.

@someC0d3r someC0d3r changed the title feat: support extending ALCops configuration feat: support remote ALCops configuration Sep 1, 2026
@someC0d3r someC0d3r closed this Sep 2, 2026
@Arthurvdv

Copy link
Copy Markdown
Member

@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?

@someC0d3r

Copy link
Copy Markdown
Author

@Arthurvdv I'm gonna reopen it then, then you can review it! :)

@someC0d3r someC0d3r reopened this Sep 3, 2026

@Arthurvdv Arthurvdv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Sequencing: configuration-load diagnostics (#328) must land first — a failure to load alcops.json or an Extends source 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.
  2. All-or-nothing fallback: when Extends is declared and cannot be fully resolved (unreachable, timeout, malformed, or the source itself declares Extends), 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.
  3. 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.json implies trusting the referenced source.
  4. 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.

@Arthurvdv

Copy link
Copy Markdown
Member

@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 🤗

@someC0d3r

Copy link
Copy Markdown
Author

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
@someC0d3r

someC0d3r commented Sep 5, 2026

Copy link
Copy Markdown
Author

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 33c2bb1, including the integration of #512 and the requirements in the maintainer review.

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:

local overrides are ignored too

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:

add a response size cap

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:

  1. Failed inheritance still applies local overrides. The provider records the failure but returns the local settings:

    if (inheritanceFailure is not null)
        localFailures = localFailures.Add(inheritanceFailure);
    
    return new ALCopsSettingsLoadResult(localSettings, localFailures);

    With a missing base file and a local CyclomaticComplexityThreshold of 41, the effective value remains 41, instead of the built-in default 8. This does not meet the requested all-or-nothing fallback. Existing tests currently assert the old behavior and need to change.

  2. The HTTP response has no application-level size cap. The resolver uses GetStringAsync, and a local reproduction accepted a valid response of 2,097,200 bytes without a failure. The five-second timeout does not enforce the requested response-size limit.

  3. Rejected URL credentials are exposed through CM0001. The credentials check correctly prevents the request, but the unmodified URL is stored as SettingsLoadFailure.Source. The diagnostic then includes its username/password in IDE output or build logs. This was reproduced using synthetic credentials through the existing CM0001 test harness.

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.

@someC0d3r

someC0d3r commented Sep 5, 2026

Copy link
Copy Markdown
Author

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:

local overrides are ignored too

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:

add a response size cap

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.

  1. Complete fallback: a failed declared Extends now returns built-in defaults and CM0001. Local overrides are discarded too, including arrays and nested settings. A local threshold of 41 with an unavailable base now correctly becomes 8.
  2. Bounded HTTP responses: HttpClient.MaxResponseContentBufferSize limits response content to 1 MiB (1,048,576 bytes) before JSON deserialization. This covers chunked responses and missing Content-Length, while retaining the five-second timeout.
  3. Credential redaction: URL username/password information is removed from the recorded diagnostic source. Credential-bearing URLs are still rejected before any connection is made, including percent-encoded credentials.

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:

SDK / analyzer target Passed Skipped Failed
AL 18 / net10.0 1,739 2 0
AL 16 / net8.0 1,723 18 0
AL 12 / netstandard2.1, tests hosted on .NET 10 1,502 239 0

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 33c2bb1 remains intact: the central CM0001 analyzer, metadata, compilation-level reporting, cached failures and regression coverage are preserved. Unknown setting names still report CM0001 while recognized settings remain effective.

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.

@someC0d3r

Copy link
Copy Markdown
Author

@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
Feel free to review it again. :)

FYI Also the docs PR got updated: ALCops/alcops.dev#161

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.

2 participants