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 |
Code review — remote ALCops configurationScope reviewed: Substantive (correctness / latency)1. Transient network failure permanently poisons a workspace's settings — 2. Synchronous blocking network I/O on the analyzer thread — Lower-severity behavioral edges3. Degenerate-but-parseable local JSON now emits CM0001 — 4. Remote fetch ignores the analyzer's CancellationToken — Cleanup5. Redundant deserialization — 6. Duplicated TFM-split JSON handling — Verified as not bugs1 MiB boundary uses We generated this review with the help of Claude. |
|
@someC0d3r awesome work and great to see GPT-6 Astra working in the wild 😄 I’ve let Claude run a code review and it came back with a few remarks we should have a look at, but overall, it looks great! With these finishing touches, I think we're good to get this into the main branch for the next ALCops release. |
Yea! Needed to try it asap 😁 I tried also other stuff with it... incredible what this model can do and in what precision.. kinda scary ngl 🥲 I'm gonna fix your mentioned points:) |
|
The follow-up to Arthurvdv's review is implemented in 7828117.
All settings consumers use the compilation captured at CompilationStart, because the SDK can expose a different object through SemanticModel.Compilation. For FC0007, the SDK's missing CompilationStart operation-registration API required registering standalone invocation syntax and binding it to the same semantic operation classifier. Its existing positive, negative and incomplete-call coverage passes across the SDK matrix. TDD: before production changes, the regression run had 7 expected failures and 138 passes. After the corrections and additional coverage, all 152 Common tests pass on every SDK. Full cop-suite results:
All seven production projects built for all three TFMs. The final formatting gate, schema JSON check and validation of all 52 internal rule guides passed. Existing MSB3277 test-reference warnings remain. These are local validation results. The README, schema descriptions and internal guidance are updated. The configuration guide and CM0001 page in docs PR #161 now explain retries, cancellation, empty local input and the remaining initial wait; Hugo Extended built 167 pages successfully. Development and validation were performed with Codex, GPT-6 Astra, reasoning 'Very High'. |
|
Circling back on the remote-config caching ( The success path is already smart 👍The durable workspace cache (keyed by app directory, process lifetime) short-circuits every later analysis pass, so on the happy path the remote URL is fetched once per process, per app directory — editing files does not re-fetch. That's the property that matters, and it's already better than AL, which re-fetches its remote ruleset on every save ( The failure path retries too oftenYour point 1 in The overlap of using a remote config and working offline is admittedly a narrow case, but it's a bad enough experience when it hits (repeated multi-second stalls while typing) that it's worth closing. Suggestion: cool down the failure retryKeep everything else as-is; just bound the retry. At the durable (per-directory) layer, cache a retryable failure for a short cooldown (~30 s) instead of not caching it at all:
Deterministic failures (invalid JSON, unknown setting, credentials) and successes keep caching as they do now. Mechanically it's an expiry alongside the memoized result in the cache entry (retryable failure → What do you think? Arthur and Claude worked together on this review. |
|
Agreed with Arthurvdv's retry-cadence review: removing process-wide failure caching allowed a new request on every compilation. Following the proposed 30-second cooldown, this is fixed in 8395257.
TDD: the new analyzer regression failed before production changes because subsequent compilations fetched again during the cooldown. After the fix, all 159 Common tests pass on each supported SDK. Coverage includes 12 subsequent analyzer runs sharing the original failure, the 29,999/30,000 ms boundary, cooldown starting after request completion, repeated failures, concurrent recovery, stable snapshots, permanent success/deterministic-error caching and cancellation during an expired retry.
All seven production projects built for all three targets. The formatting gate, schema JSON check and validation of 52 internal guides passed. Existing MSB3277 test-reference warnings and NU1900 warnings while retrieving NuGet vulnerability metadata remain. These are local validation results. The first uncached request and each eligible retry can still wait for the five-second timeout; the cooldown prevents a new request on each editor pass during an outage. There is no background refresh. The README, schema, internal guidance and docs PR #161 now describe this behavior. Hugo Extended built 167 pages successfully. Development and validation were performed with Codex, GPT-6 Astra, Reasoning: Very High. |
|
@someC0d3r this turned out really nice, thank you for sticking with me and working through all the feedback (and shout-out to Codex too 😄) A remote |
|
@someC0d3r, oops, the CI/CD is failing where I’m seeing random versions fail. My experience tells me we might be running into a concurrency issue, as analyzers run in parallel. We’re almost at the finish line, would you mind taking a look at this last issue? |
|
@Arthurvdv No worries, uno momento :) |
|
Fixed the intermittent CI failure reported by Arthur in 00115d5. I inspected all 50 SDK test reports from the failing run. All 11 failures were the same test, The test launched 12 synchronous lookups with The competing callers now use dedicated threads with Red/green evidence: in an isolated process with four worker threads and queued server continuations, the old callers stalled until the watchdog released the pool and reproduced the connection failure. The corrected test passed under the same constraints without watchdog intervention. All 159 Common tests passed on AL 12, AL 16 and AL 18 with two logical processors, followed by 25 constrained-pool repetitions per SDK (75 total). Formatting and validation of all 52 internal guides passed. GitHub CI: the new run for this commit is currently This correction changes the test harness and internal testing guidance. The production cache, 30-second cooldown, five-second HTTP timeout and assertion coverage remain intact; no public documentation change is needed. Development and validation were performed with Codex, GPT-6 Astra, Reasoning: Very High. |
Happy to help! :) thx for all the thoughtful feedback, it’s helped make the implementation much more robust. |
|
Awesome, let's get this merged! 🎉 |
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.
HTTP failures use a 30-second cooldown per workspace, allowing recovery within the session while avoiding repeated downloads during offline editing. Cancellation propagation, consistent compilation snapshots and shared JSON handling keep settings and diagnostics aligned.
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
CI follow-up: the concurrent HTTP recovery test now uses dedicated callers and a bounded start barrier, avoiding starvation of its in-process server on small runners. The production cache and timeouts are unchanged. All 159 Common tests passed on AL 12/16/18 with two logical processors, plus 75 isolated repetitions with a four-worker pool; the old task scheduling reproduced the failure under the same constraints.
The cooldown regression test failed before production changes: subsequent compilations fetched again during the intended cooldown. After the correction and deterministic clock coverage, all 159 Common tests pass on each SDK. Tests advance an isolated monotonic clock without real cooldown sleeps or modifying global clock state.
dotnet format --verify-no-changes, schema JSON validation and.claude/scripts/Validate-Rules.ps1passed; the latter checked 52 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.