Rewrite the Copilot instructions as PR-review guidance - #310
Merged
Conversation
This was referenced Aug 27, 2026
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
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.
Problem
The
copilot-instructions.mdfiles were generated by Copilot's repository onboarding feature inNovember 2025 and have not been maintained since. Two things are wrong with them.
They are the wrong genre. They describe what the repository is: directory trees, dependency
lists,
pip install black, "how to set up the environment". None of that helps a reviewer readinga diff. What a reviewer needs is what cannot be deduced from the diff — the invariants this
codebase silently violates, the failure signatures they produce, and what is already checked
elsewhere so it is not worth a comment.
Much of the content is now false. Checked against the current tree:
.github/integration_cfg.yamlholds the pipeline config / authorised userscms-flaf/FLAF_ciconfig/<era>/samples.yamldatasets.yaml;samples.yamlsurvives only in the Run 2 legacy erasPlotKitis a submoduleHH_bbWW/Analysis/tasks.pyStale facts are worse than no facts: the old files end with "Trust These Instructions. Only search
the codebase if information appears incomplete or outdated."
Solution
Rewritten as review guidance. Each file now leads with what a useful comment looks like here,
then the invariants worth checking, then — deliberately — what not to comment on.
The material is not invented. Every invariant listed is one that caused a real incident and is
recorded in the framework documentation or a merged fix, and each is stated with its failure
signature so a reviewer can recognise it in a diff. Copilot has already caught one of these
classes unaided (the shared
workflow_conditionobject, #293); the aim is to make that the normrather than luck.
FLAF carries the shared framework invariants — law task semantics, bundles, the remote-path
cache, processor stages, concurrency — and the analysis repos point at it rather than duplicating,
mirroring how the documentation is organised. That is why the analysis files got substantially
shorter while FLAF's grew.
A "do not flag" section is included on purpose. The main cost of an automated reviewer is noise,
and most of the noise here is predictable: comment density, PyROOT idioms, deliberate per-era
config duplication, requests for unit tests of code that needs CVMFS and a grid proxy, and
anything the formatting workflow already settles.
Every path and filename referenced was verified to exist, and the facts tables are dated so the
next reader knows how much to trust them.
Testing
Documentation only — no code change. Verified:
(
upstream_workflow,workflow_condition,source_hash/task_requires,stages,Setup.get_processors,resolveNegativeBins).No docs change: the published MkDocs sites are unaffected — this file is repository metadata for
review tooling.
What this file now carries
It is the anchor: the four analysis/Corrections files defer to it. Sections are the law semantics
that bite (
workflowpropagating throughreq(), the sharedworkflow_condition, theexisting-branch snapshot, the
poll()job-count snapshot), bundles (why an unhashed code bundleis never rebuilt, why every packed producer must be in
task_requires, why a preserved symlinkstill reaches AFS), the remote path cache (absence may only be inferred from a listing marker; the
cost of a change at 10k jobs; read-after-write lag), processors and stitching, the CWD temp-file
race, the config layering rules, and the expectation that a fake calls the real
__init__.Also recorded: CI runs only
test_setup_loading.py, so a broken pytest suite intest/is notcaught automatically — which is how the path-cache suite stayed red through a merge cycle (#309).