ci: grade only open pull requests against the linked-issue policy - #125
Kartikey1306 wants to merge 7 commits into
Conversation
d0329e5 to
cbc064b
Compare
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
cbc064b to
95e441b
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#125 "ci: grade only open pull requests against the linked-issue policy"
head: 95e441b author: Kartikey1306 ci: pending
Verdict: The one commit this PR owns (95e441b, .github/workflows/linked-issue.yml, +6/−0) is correct and does not weaken the gate — it adds if: github.event.pull_request.state == 'open' to the policy job so a body edit on a merged PR stops grading a tree nobody can change. I verified the premise and the structure myself rather than taking the PR body's table on faith. Two things are worth fixing before this lands: the same caller is byte-identical in 19 repositories, not the three this batch touches, and the PR's stated review range names a commit that does not exist.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/linked-issue.yml:14 |
The guard is being pasted into 3 of the 19 repos that carry this caller. I hashed origin/master:.github/workflows/linked-issue.yml across the working root: 19 repos, all sha256:120e9f40a521…, byte-identical (eAI, eApps, eBoot, eBrowser, ebuild, eCAD-Hardware-Products, eDB, eFirmware, eIPC, embeddedos-org, embeddedos-org.github.io, eNI, eOffice, eos, eos-aero, EoSim, eosllm, EoStudio, www.embeddedos.org). The other 16 keep grading merged and closed PRs on every body edit, which is the defect this PR exists to remove. The PR body says "the caller is byte-identical in each" of three repositories — it is byte-identical in nineteen. |
Put the guard on jobs.linked-issue in the shared reusable workflow embeddedos-org/.github/.github/workflows/linked-issue-policy.yml instead. A called reusable workflow evaluates github.event.* against the caller's event, so if: github.event.pull_request.state == 'open' works unchanged there and fixes all 19 consumers from one place. The uses: pin is already propagated org-wide by Dependabot (6 open bump PRs today), so the pin bump is mechanical. If the guard must stay in the caller instead, say so here and open the remaining 16 PRs — leaving 16 repos on the broken behaviour is not a finished fix. |
| 2 | Medium | PR scope (not the commit) | This PR is stacked on #115 and mergeable is MERGEABLE. Merging it as it stands lands #115's diff too: 26 files, +708/−191, including core/sha512.c (+211/−111), core/ed25519_verify.c (−10) and the deletion of tests/unit/test_ed25519.c (−40, whole file) — cryptographic code in the repo the master design requires to keep its trusted computing base minimal and auditable (§5.1, §8) — under a ci: title with a CI-only review. Nothing mechanical prevents that; only mergeStateStatus: BLOCKED does. |
Do not merge before eBoot#115. Land #115, rebase, and let this drop to its one commit. #115's content was reviewed by this pipeline at this exact head — see reports/eBoot-115-a276016b.md; it is not re-reviewed here. |
| 3 | Low | PR body, line 1 | "Review a276016..cbc064b for this change" — cbc064b does not exist in this repository (git cat-file -t cbc064b → Not a valid object name). The head is 95e441b. A reviewer following the instruction literally gets an error. The same defect is in ebuild#144 (3708afb) and eos#154 (7a1d1e7); none of the three end shas resolve in any of the three repos. |
Use a276016..95e441b, or drop the sha and say "the single commit at the head of this branch". |
| 4 | Low | PR body, Verification table | Four rows read PASS with no command output, which .ai/reviewer.md and the review brief (§5) treat as the finding regardless of whether the claim is true. Recorded for the missing evidence only — I re-ran all four and they hold, so this is not a wrong claim: see "Not checked" below for what I ran. |
Paste the command and its output, not a table of verdicts. |
Architecture conformance
Conforms. Master design §21 places CI templates in the Infrastructure tier, and this change is a workflow-level if on a governance check — it adds no #include, no link line, no manifest entry and no runtime dependency, so §5.1's direction-of-dependency law is untouched and eBoot's TCB surface is unchanged by 95e441b itself. The one architectural tension is Finding 1: §21 assigns CI templates to Infrastructure, yet this puts policy semantics into 19 product-repo callers where they can drift. That gap in the design text is written up as a proposal in proposals/2026-09.md (2026-09-14 — "Shared CI policy: the caller/reusable-workflow contract is undefined").
Proposed changes
Smallest sequence that keeps everything working:
- Move the guard up one level, in
embeddedos-org/.github:then bump thejobs: linked-issue: name: Policy / Linked Issue # pull_request_target fires on `edited` for closed and merged PRs too. # A merged PR cannot be changed to satisfy the policy. if: github.event.pull_request.state == 'open' runs-on: ubuntu-latest
uses:pin in the callers. This is behaviour-identical for open PRs in all 19 repos and removes the copy in each. - Keep this PR's caller-side guard only if (1) is rejected — and then open the other 16.
- Fix the review range in the body to
a276016..95e441b. - Merge #115 first; rebase this onto it so the PR is its one commit.
Not checked
- CI is pending, not failing. At bundle time (6 minutes after the PR opened)
Build & Test (Linux x86_64),Host Build & Tests,Fuzz Harness Build,Cross-compile STM32F4,Analyze (C/C++)andEoSim arm64-linuxwere allpendingwith 0s elapsed. I did not wait for them and I am not calling them a pass or a failure.policy / Policy / Linked Issueitself passed (5s).mergeStateStatus: BLOCKED. - I did not execute the workflow. No GitHub Actions run can be triggered from here, so the guard's runtime behaviour on a real
edited-on-merged event is unverified by execution. What I did verify:- Structure — parsed the file at
95e441bwithyaml.safe_loadand asserted on the parsed document:jobsis exactly['policy'],ifis"github.event.pull_request.state == 'open'"and sits on the job,usesstill pins@92cb596c…,with.policy_refunchanged, trigger types still[opened, edited, reopened, synchronize],permissionsunchanged. PASS on eBoot, ebuild and eos; all three resulting files hash identically (fafeb8d17c7b). - The premise —
gh run list --workflow "Linked issue policy" --status failurereturns exactly 13 runs, allpull_request_target, all within 2026-09-14T20:25–20:26Z. I resolved every head branch to its PR and checked its state: #58 CLOSED, #59 MERGED, #77 CLOSED, #80 MERGED, #81 MERGED, #82 MERGED, #84 MERGED, #85 MERGED, #86/#92 MERGED, #88 MERGED, #89 MERGED, #97 CLOSED, #99 MERGED. All 13 are merged or closed. The PR body's central factual claim is correct. - Not redundant with the upstream bump — the newer policy commit
186aef0dthat Dependabot is proposing org-wide contains no state guard either, so this fix is not already upstream.
- Structure — parsed the file at
- Effect on branch protection not verified. I did not read branch-protection settings, so whether
policy / Policy / Linked Issueis a required context — and therefore how a skipped job is scored — is unconfirmed. It does not affect open PRs, where the guard is true and the job runs as before. - #115's content is not re-reviewed here, per the brief's rule against repeating a point already made; see
reports/eBoot-115-a276016b.md.
Automated architecture review of 95e441b396e1 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
|
Finding — the guard belongs in the reusable job, not in 3 of 19 callers: agreed, and done at the source: embeddedos-org/.github#9 puts Stacked-merge risk: agreed that only The review range: the SHAs stopped resolving because the branch was re-triggered with a no-content push after the body was written. The body now says "the single commit at the head of this branch", which survives that. Verification table without output: replaced with the commands and their output. Nothing pushed to this branch; the content is the same one commit. |
|
All four findings addressed; no push to this branch, since every one was about the body or about scope, and a push here would re-stack nothing but would dismiss any approval. Head stays 1 (Medium, 19 repos) — done the way you recommended: embeddedos-org/.github#10 puts 2 (Medium, scope) — the second paragraph of the body is now the merge-order statement in the merger's terms: what merging as-is lands (26 files, the crypto ones named), and what not to do (merge before #115). 3 (Low, 4 (Low, evidence) — the verification table is replaced by the commands and their output, measured at Nothing pushed, so nothing to re-approve. |
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/[email protected] pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
… -1.0.0 as an option test_out_of_range_version_is_rejected[-1.0.0] passed locally (Python 3.14) and failed in CI (ubuntu-22.04, Python 3.10) with argparse's own "expected one argument": the older negative-number matcher does not accept -1.0.0, so the token was read as an unknown option and imgpack.py's range check -- the thing under test -- never ran. The joined form is unambiguous on every interpreter and the test now reaches the tool's message.
tests/vectors/fw_update_test_sigs.h is the output of tools/gen_fw_update_test_sigs.py, committed because eBoot has no Ed25519 signer in C. Nothing checked that the two agree: a generator edit without a regeneration leaves test_fw_update and test_fw_transport verifying against stale signatures, failing with EOS_ERR_SIGNATURE and nothing to say why. tests/unit/test_fw_update_test_sigs.py runs the generator with the test's own interpreter and compares its stdout to the committed header byte for byte, so a line-ending change counts too. It follows the same dependency rule as test_eos_sign_payload_offset.py: with EOS_REQUIRE_SIGNING_TESTS set (the CI workflow sets it before the pytest step) a missing cryptography module fails the job instead of skipping. Negative control: one flipped hex byte in the header fails the test with a unified diff naming the line. build_image() and build_container() now carry a comment naming the coupling: the signed prefix is assembled both there and in the generator, and changing any field in it means changing the generator's copy and regenerating the header.
…epair embeddedos-org#103 and embeddedos-org#104 were both merged and disagree on whether the anti-rollback counter or the signature is checked first in eos_fw_update_finalize(). The master design orders boot as verify image, then version policy (section 8.1), but its update flow (section 15) never places the anti-rollback check, so the order the install path uses existed only in a PR body. ADR-020 records it: the signature over the signed header prefix is verified first, the TLV counter is read only after the prefix that binds it is authenticated, and an image that fails verification is refused as EOS_ERR_SIGNATURE without its counter being consulted. docs/adr/README.md is added in the shape of the eos repository's index; 020 avoids reusing 001 through 019. CHANGELOG.md gains the Unreleased entries for the repair: the configure, compile and link breakage after the 09-07 batch merge, the settled check ordering with the suites streaming signed images, the re-derived Valgrind list, fuzz-build in the CI gate, counted tests_run, the EoSim Windows legs running under bash, and the Scorecard action on its ghcr.io-hosted release.
The changelog said eleven suites had no Valgrind run. Eleven were missing from EBLDR_UNIT_TESTS, but four of those were named in the hand-written foreach and did run; seven had no run at all. Say which. ADR-020: its design-document citation now says where it comes from (the architecture review of embeddedos-org#115), and the sentence about the two PRs' bases is replaced with what the history shows -- embeddedos-org#103's commits predate embeddedos-org#104's merge, and embeddedos-org#104 was written without embeddedos-org#103's check in place.
The regeneration test's failure path -- the unified diff under the regenerate command -- ran only when the header was stale, so a green run never executed it and the coverage report said so. The report is now a helper the match asserts with, and a second test drives the helper with two byte strings that differ in one byte and checks the command and both sides of the changed line appear.
pull_request_target fires on `edited` for closed and merged pull requests as well as open ones, and the caller had no guard on the PR's state. So a body edit on a PR merged before this policy existed graded it against the policy and failed it: on 2026-09-14 that produced 13 failing "Linked issue policy" runs in this repository, every one on a merged or closed branch. A merged PR cannot be changed to satisfy the policy, and grading it says nothing about the tree. Only an open PR is a decision still to be made, so the job now runs only when github.event.pull_request.state == 'open'. The open PRs in the org that fail this policy today (other authors) are unaffected: they are open, so the guard is true and the policy still runs. Verified: the workflow parses and the parsed structure is what was intended -- the `if` sits on the `policy` job, `uses` is unchanged, and the trigger types are still [opened, edited, reopened, synchronize]. Checked with an assertion on the parsed document, not by parse success alone. Closes embeddedos-org#124
95e441b to
f205a78
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#125 "ci: grade only open pull requests against the linked-issue policy"
head: f205a78 author: Kartikey1306 ci: pass (26 green, 0 red; only Create GitHub Release skipped)
Verdict: Follow-up review. This head is a pure re-stack onto #115's rebased tip — the
single commit is byte-identical by git patch-id. All four findings from the previous review
are answered, three by edits to the PR body and the important one by doing the fix at the right
level upstream. No new findings in the diff. What this PR now needs is not a change — it is
blocked on a maintainer in another repository, and that is the substance of this review. One
correction to my own earlier count, below.
Status of the previous findings (eBoot-125-95e441b3.md)
| # | Prev. severity | Status | Evidence |
|---|---|---|---|
| 1 | Medium — the guard is pasted into 3 of the repos that carry this caller, leaving the rest on the broken behaviour | Addressed at the right level, and now BLOCKED there. | embeddedos-org/.github#9 puts the guard on jobs.linked-issue in the reusable workflow, which is exactly the recommended fix, and the author closed their own duplicate #10 in its favour. I checked its state directly: #9 is still OPEN, MERGEABLE, isDraft:false, and untouched since 2026-09-14T21:08:58Z — over 26 hours with no maintainer action. Neither the author nor this pipeline has write access to embeddedos-org/.github, so nothing on this side can advance it. Until it lands and the callers' pins bump, this repository's caller-side line remains the only guard it has, which is what the PR body now says. Recorded as open and externally blocked, not resolved. |
| 2 | Medium — merging this as it stands lands #115's 26-file diff under a ci: title |
Resolved. | The merge-order statement is now the second paragraph of the body, before the diff, in the merger's terms: what merging as-is would land (26 files, +708/−191, core/sha512.c and core/ed25519_verify.c named) and the instruction not to merge before #115. The author is right that nothing mechanical enforces it and that the durable fix is a merge-queue rule, not a per-PR one. |
| 3 | Low — the body's review range named cbc064b, which does not exist |
Resolved, twice over. | Corrected to a276016..95e441b with the correction left visible, then corrected again to e152d8e..f205a78 after this re-stack. Both current SHAs resolve; I checked. |
| 4 | Low — four verification rows read PASS with no command output | Resolved. | The table is replaced by the two commands and their actual output (the yaml.safe_load assertion and the gh run list count), and the body carries an explicit NOT RUN line for the live edited-on-merged event. That is the shape .ai/reviewer.md asks for. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| — | — | — | None in the diff. | — |
The commit is +6/−0 in one file, five of which are a comment. The one open item is finding 1
above, which is not a defect in this diff and cannot be closed from this repository.
Correction to this pipeline's own earlier number, not a finding against the author. The
previous review stated the caller is byte-identical in 19 repositories and the PR body repeats
that figure. Re-measured across every repository in the working root: it is 20 — the same
sha256:120e9f40a521…, none of them carrying the guard, and the one the earlier list omitted is
eos-health, which has had the file since 65ca89e (2026-09-10), before that review. So the
scope of finding 1 is one repository wider than reported, not narrower, and the number is
growing as repositories are added — which is the argument for fixing it upstream rather than
per-caller, made slightly stronger.
Verification performed for this review
No build was needed; this PR changes one YAML file and nothing that compiles. Read-only work
against the local clone and the GitHub API. The user's eBoot checkout was not touched; nothing
was committed or pushed.
| Check | Result |
|---|---|
| Is this head new work or a re-stack? | RE-STACK, confirmed. One own commit at each head; git patch-id --stable of 95e441b3 and f205a78e are identical. The parent moved a276016 → e152d8e, which is #115's own rebase. |
Structural diff of .github/workflows/linked-issue.yml, base vs head, on the parsed document |
Exactly one change. Changed job keys: {'if': "github.event.pull_request.state == 'open'"}. Removed job keys: none. jobs is still exactly ['policy']; uses still pins @92cb596c…; with.policy_ref unchanged; permissions {contents: read, issues: read, pull-requests: read} unchanged; trigger types still [opened, edited, reopened, synchronize]; no secrets: added. The if is on the job, not on a step — a step-level if would still start the job and report a status. |
embeddedos-org/.github#9 state |
OPEN, MERGEABLE, not draft, updatedAt 2026-09-14T21:08:58Z. #10 is CLOSED, as the author says. |
| Caller fan-out, re-measured | 20 repositories, all sha256:120e9f40a521…, guard=0 in every one: eAI, eApps, eBoot, eBrowser, eCAD-Hardware-Products, eDB, eFirmware, eIPC, eNI, eOffice, EoSim, EoStudio, ebuild, embeddedos-org, embeddedos-org.github.io, eos, eos-aero, eos-health, eosllm, www.embeddedos.org. |
Previous review's open item: is policy / Policy / Linked Issue a required check? |
ANSWERED — no. gh api repos/embeddedos-org/eBoot/branches/master/protection returns required_status_checks: null. So a skipped job cannot block a merge here, and the question of how branch protection scores a skipped required context does not arise in this repository. (It is also moot for the guard's actual effect, which only skips on closed and merged PRs.) |
| Previous review's open item: CI was pending, not judged | ANSWERED — green. checks.txt at this head reports 26 checks passing, 0 red, with only Create GitHub Release skipping. Read from the bundle; I did not re-query GitHub or open any job log. |
| Was anything weakened? | No. The parsed-document comparison above is the evidence: nothing was removed, no permission widened, no trigger dropped, no pin loosened. For an open PR the guard evaluates true and the policy runs exactly as before. |
Architecture conformance
Conforms. §21 places CI templates in the Infrastructure tier, and this is a workflow-level
if on a governance check: it adds no #include, no link line, no CMake entry, no manifest
dependency and no runtime dependency, so §5.1's direction-of-dependency law is untouched and
eBoot's TCB surface is unchanged by f205a78 itself. The architectural tension is unchanged and
is finding 1's: §21 assigns CI templates to Infrastructure, yet the policy's semantics live in 20
product-repo callers where they can drift independently. That gap in the design text is already
written up — "Shared CI/governance policy: the caller/reusable-workflow contract is undefined"
(2026-09-14) in .ai/autoreview/proposals/2026-09.md. No new proposal is appended; the
existing one covers this head, and the only new fact — 20 callers rather than 19 — strengthens it
without changing what it asks for.
Blocked / stale status
Stating this plainly because the brief asks for it: this PR is not stale and is not failing CI
— it is green and its one commit is correct. It is waiting on two things, neither of which the
author can do:
- #115 must land first, or merging this lands #115's diff under a
ci:title (previous
finding 2, now documented rather than mechanically prevented). embeddedos-org/.github#9needs a maintainer with write access. It has sat OPEN and
MERGEABLE for 26+ hours. Until it merges and the callers'uses:/policy_refpins bump, 19
other repositories keep grading merged and closed PRs on every body edit. When it does merge,
this PR becomes redundant and should be closed, not merged — the body already says so.
What would unblock it: a maintainer review on embeddedos-org/.github#9, then a pin bump (which
Dependabot already propagates org-wide).
Proposed changes
Nothing to change in this diff.
Sequencing:
1. merge #115
2. then either
a. embeddedos-org/.github#9 merges -> bump the pin here -> CLOSE this PR, or
b. #9 stays blocked -> rebase and merge this as the interim single-repo guard
Not this PR's job, recorded so it is not lost:
the same one-line guard is missing in 19 other repositories, eos-health included.
Not checked
- I did not execute the workflow. No GitHub Actions run can be triggered from here, so the
guard's runtime behaviour on a realedited-on-merged event is unverified by execution. The
evidence is structural (the parsed-document comparison above) plus the 13 real failures the
previous review resolved to merged/closed PRs. - The 13 failing runs were NOT re-queried at this head. That measurement is from the previous
review; the premise has not changed and the commit is patch-identical, but the count is not
re-observed today. - I did not read
embeddedos-org/.github#9's diff or its tests this round — only its state,
mergeability andupdatedAt. The author's description of its contents and mutation testing is
taken at face value here; it is a different repository and outside this bundle. - Dependabot pin propagation is Inferred, not observed: I did not check for open bump PRs on
this head. - #115's content is not re-reviewed here, per the brief's rule against repeating a point
already made; seereports/eBoot-115-e152d8ed.md. - No build or test run. This PR changes one YAML file and compiles nothing;
ctest/pytest
results for the underlying tree are in the #115 report at the same base.
Automated architecture review of f205a78e3b6e — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
|
All four findings recorded as resolved in your review of |
This branch has not been deployed
3 participants
Closes #124
The real fix is one level up — embeddedos-org/.github#9
The review of this PR hashed the caller across the org: it is byte-identical in nineteen repositories, not the three this batch touched, and the right place for the guard is the shared reusable workflow, where the
githubcontext is the caller's and one line covers every consumer once its pin bumps. That PR is open — #9, opened by a parallel session one minute before my identical #10, which I closed in its favour; #9 also carries a structural test that the guard sits on the job. Neither of us has write access toembeddedos-org/.github, so it is a fork PR awaiting a maintainer.Until #9 lands and pins bump, this caller-side line is the only guard this repository has. Once they do, this PR (and ebuild#144, eos#154) becomes redundant and should be closed rather than merged. I have not opened the other sixteen caller-side PRs: sixteen copies of a one-line workaround, each needing its own review, is the wrong shape when the one-line real fix is already open.
Problem
.github/workflows/linked-issue.ymlruns onpull_request_targetfor[opened, edited, reopened, synchronize]and has no guard on the pull request's state.editedfires on closed and merged PRs too, so a body edit on a PR merged before the policy existed grades it and fails it.On 2026-09-14 that produced 13 failing
Linked issue policyruns in this repository, every one on a merged or closed branch. A merged PR cannot be changed to satisfy the policy, and grading it says nothing about the tree.Change
One line on the job:
if: github.event.pull_request.state == 'open', with a comment saying why.What it does not change: the open PRs in the org that currently fail this policy are open, so the guard is true and the policy keeps running on them exactly as before. Nothing is weakened for any PR that can still be acted on.
Verification, at
95e441b(re-run unchanged atf205a78— the tree is byte-identical, only the base moved)Each of those 13 runs' head branch was resolved to its PR and its state checked: all merged or closed. The
ifis on thepolicyjob;usesand the trigger types are unchanged — asserted from the parsed document above, not from parse success.NOT RUN: a live
editedevent on a merged PR against this branch.pull_request_targetruns the workflow from the base branch, so a PR cannot exercise its own change to it; the 13 real failures are the evidence the guard is aimed at the right condition.