Skip to content

Rewrite the Copilot instructions as PR-review guidance - #310

Merged
kandrosov merged 2 commits into
cms-flaf:mainfrom
kandrosov:copilot-review-instructions
Aug 28, 2026
Merged

Rewrite the Copilot instructions as PR-review guidance#310
kandrosov merged 2 commits into
cms-flaf:mainfrom
kandrosov:copilot-review-instructions

Conversation

@kandrosov

Copy link
Copy Markdown
Contributor

Problem

The copilot-instructions.md files were generated by Copilot's repository onboarding feature in
November 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 reading
a 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:

claim reality
.github/integration_cfg.yaml holds the pipeline config / authorised users that file does not exist in FLAF, HH_bbtautau, HH_bbWW or H_mumu; the workflow reads cms-flaf/FLAF_ci
config/<era>/samples.yaml Run 3 uses datasets.yaml; samples.yaml survives only in the Run 2 legacy eras
RunKit is a submodule vendored since #272; only PlotKit is a submodule
4 GitHub workflows there are 8 in FLAF, 5 in each analysis
LCG_107_cuda, ~2 MB, ~13k lines of Python LCG_108a, 7.5 MB, ~24k lines
dataset consistency runs Run3_2022…Run3_2025 it runs through Run3_2026
HH_bbWW/Analysis/tasks.py does not exist

Stale 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_condition object, #293); the aim is to make that the norm
rather 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:

  • every file path and bare filename mentioned resolves in the current tree;
  • the workflow lists, era lists, submodule lists and config filenames match the repository;
  • the claims about framework behaviour were checked against the code they describe
    (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 (workflow propagating through req(), the shared workflow_condition, the
existing-branch snapshot, the poll() job-count snapshot), bundles (why an unhashed code bundle
is never rebuilt, why every packed producer must be in task_requires, why a preserved symlink
still 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 in test/ is not
caught automatically — which is how the path-cache suite stayed red through a merge cycle (#309).

@kandrosov
kandrosov merged commit 593bc11 into cms-flaf:main Aug 28, 2026
5 checks passed
@kandrosov
kandrosov deleted the copilot-review-instructions branch August 28, 2026 06:38
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.

1 participant