Repository navigation
Conversation
Nine PRs were merged into master within minutes on 09-08, each on the base it was written against. Master has not compiled since, and the Python guards that would have named the rest never ran because the C build failed first. Compile (every C job, CodeQL, and the ARM64 kernel in the simulation): - services/linux/src/linux_security.c: embeddedos-org#119 and embeddedos-org#132 each added an #else to the same #ifndef _WIN32 in eos_busybox_install_to_rootfs(), so master has "#else after #else". The embeddedos-org#132 arm (`(void)bb;`) is the one removed -- the embeddedos-org#119 arm already uses bb and reports the unsupported platform. Guards from embeddedos-org#121 / embeddedos-org#93 that later merges walked back: - ci.yml: embeddedos-org#132 added windows-test after embeddedos-org#121's gate; the gate did not wait for it, so "CI Gate" could be green with the MSVC leg red. - test_ci_gate.py: embeddedos-org#129 gave eosim-sanity.yml and simulation-test.yml a path-filtered pull_request trigger (they test their own edits); a path-filtered check cannot be required, so both are recorded in NOT_REQUIRED with the book-build.yml reason. - tests/CMakeLists.txt: test_linux_security_paths (embeddedos-org#119) and test_pkg_fetch (embeddedos-org#115) had no add_executable(). embeddedos-org#115's replay replaced embeddedos-org#119's block with its own, and embeddedos-org#118's replay replaced that; two suites compiled against nothing. Both registered again. 41 -> 43 suites. - tests/test_kernel.c: four tests from embeddedos-org#130 and embeddedos-org#131 were defined and never called -- their RUN() lines did not survive the replay of main(). And the one that was not bookkeeping: - kernel/src/task.c: embeddedos-org#130 was merged after embeddedos-org#131 from a base that predates it, and its copy of task.c replaced embeddedos-org#131's. embeddedos-org#131 had also flattened 574 CRLF line endings, so its 1166-line diff hid a 39/21 change and the replay took embeddedos-org#130's side wholesale. Master kept embeddedos-org#131's tests and lost its kernel: the idle task could be deleted and suspended, a half-initialised TCB was published to the scheduler before its stack existed, and eos_schedule() pointed g_current_sp at the outgoing task. test_idle_task_is_permanent fails on master the moment it is called. embeddedos-org#131's task.c diff re-applied on top of embeddedos-org#130's; the result is embeddedos-org#131's file plus embeddedos-org#130's wake_armed hunk and nothing else (verified by diff against 3a00bd9). Verified locally (macOS, clang): Release build clean, 43/43 ctest; the README default configuration builds; 47/47 pytest. Not in this PR: the nightly "Upstream drift" job builds eBoot at master and eBoot master is broken separately (its own repair PR); bump EBOOT_COMMIT in eos-simulation.yml once that lands.
Re-registering it put it in front of the MSVC leg (embeddedos-org#132) for the first time, where <unistd.h> does not exist. The suite probes with access(), open(O_CREAT) and rmdir to prove refused inputs never reach a shell; on Windows the library's own #else arm refuses every install/build call before is_path_safe() matters, so there is nothing for it to test there.
… gives 1 MB Re-registering the POSIX-only suite let the MSVC leg reach ctest for the first time, and test_firmware died there with SEGFAULT: five tests declare a stack EosConfig (sizeof 3,953,236 on this ABI), which the 8 MB default stack on Linux and macOS absorbs and MSVC's 1 MB does not. test_config.c already keeps its EosConfig static for the same reason; the same here.
kernel.h declared eos_task_delete() and eos_task_suspend() without saying what handle 0 is. It is the idle task, it is permanent, and both calls return EOS_KERN_INVALID for it -- as they do for an out-of-range handle or an empty slot. The doc comments now say so, in the header's Doxygen style. eos_schedule() has an early return for "no runnable task" that is unreachable by construction: slot 0 cannot be deleted or suspended, the idle task only spins or waits for an interrupt and so never blocks, and find_next_task() can select it because of the "best < 0 ||" term. The comment records that reasoning, and that the return is self-healing if it were ever taken: the outgoing task stays READY and the next eos_schedule() marks it RUNNING. task.c also carried two stray blank lines at EOF from the re-apply of embeddedos-org#131; with them gone, the diff against 3a00bd9 is embeddedos-org#130's wake_armed hunk plus this comment and nothing else.
…rootfs is_path_safe() refuses NULL, but eos_busybox_install_to_rootfs() read bb->source_dir to hand it to the predicate, so a NULL bb reached the dereference before the guard could see it: a crash rather than the -1 every other refusal in this file produces. Both arguments are now checked first, before either is read. test_linux_security_paths gains a case for it: (NULL, dir) and (&bb, NULL) both return -1, and the directory gets no /init. With the guard removed and the target rebuilt, the suite dies with SIGSEGV (exit 139) on the first call; with it restored, the suite passes.
Records under Unreleased what embeddedos-org#144 did and why: the "#else after #else" that stopped master compiling after the 09-08 batch merge, the guards the replays walked back (two suites re-registered, four test_kernel tests wired into main(), windows-test in the CI gate, the two path-filtered simulation workflows classified), and the NULL guard in eos_busybox_install_to_rootfs(). The scheduler change gets its own Changed entry because it is a contract: eos_task_delete(0) and eos_task_suspend(0) return EOS_KERN_INVALID again, as embeddedos-org#131 specified before embeddedos-org#130's replay replaced its task.c. Changed sits above Fixed, the order the older releases in this file use.
…end checks The eos_schedule() comment claimed the early return was self-healing. It is not: find_next_task() scans every slot including the outgoing one, so next < 0 means nothing is READY, and the return leaves the state exactly as the canary check left it. The comment now says so, and that recovery is a fault handler's job rather than the scheduler's. eos_task_suspend() refuses a slot whose entry is NULL, which is what the doc comment now says instead of the looser "holds no task"; a slot the canary check marked DELETED still has its entry set and is accepted. tests/CMakeLists.txt: the reason test_linux_security_paths is POSIX-only is that the shell-building bodies are compiled out on Windows, not that the predicate is skipped there.
Kartikey1306
force-pushed
the
ci/linked-issue-policy-open-only
branch
from
September 14, 2026 20:33
6521844 to
7a1d1e7
Compare
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 26 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#153
Kartikey1306
force-pushed
the
ci/linked-issue-policy-open-only
branch
from
September 14, 2026 20:37
7a1d1e7 to
0a2a706
Compare
srpatcha
reviewed
Sep 14, 2026
srpatcha
left a comment
srpatcha
left a comment
Member
There was a problem hiding this comment.
Review — eos#154 "ci: grade only open pull requests against the linked-issue policy"
head: 0a2a706 author: Kartikey1306 ci: pending
Verdict: The one commit this PR owns (0a2a706, .github/workflows/linked-issue.yml, +6/−0) is correct and does not weaken the gate — if: github.event.pull_request.state == 'open' stops a body edit on a merged PR from grading a tree nobody can change, and leaves the open-PR path untouched. But this repository's copy of the PR body carries a verification row that is false: I resolved all 26 runs and one of them is on an open PR. The claim, not the code, is the problem here.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | PR body, Problem §2 and Verification row 3 | "26 failing Linked issue policy runs in this repository, every one on a merged or closed branch", asserted PASS as "each run's head branch resolved to its PR and its state checked". I did exactly that resolution for all 26 runs on 2026-09-14. 25 are merged or closed; the 26th is not. The run at 2026-09-14T01:51:59Z is on autofix/secureboot-null-path-crashes-before-its-own-guard, which is eos#146 — OPEN right now, and its policy / Policy / Linked Issue check is still fail today. The body's own Change section contradicts its Problem section on this point (it concedes open PRs in eos and ebuild currently fail the policy). Per .ai/reviewer.md, an unsupported "verified" is itself the finding; here it is worse than unsupported, it is contradicted. This does not invalidate the change — the guard correctly leaves eos#146 graded, because it is open — but the stated evidence is wrong. |
Restate the count as "25 of 26 on merged or closed branches; the 26th is eos#146, which is open and fails the policy on its merits", and paste the resolution command and its output. Fix eos#146 separately by adding a closing issue reference to its body. |
| 2 | 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 — the exact 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 the expression 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 caller-side, say so here and open the remaining 16. |
| 3 | Medium | PR scope (not the commit) | Stacked on #144, and mergeable is MERGEABLE. Merging as-is lands #144's diff too: 11 files, +148/−31, including kernel/src/task.c (+53/−23) and kernel/include/eos/kernel.h (+20) — the scheduler/task surface the master design freezes as a boundary (§6, §6.1, §23.2 "EoS API — stable source-level application API within a major release") — plus services/linux/src/linux_security.c, under a ci: title with a CI-only review. Only mergeStateStatus: BLOCKED prevents that; nothing mechanical does. |
Do not merge before eos#144. Land #144, rebase, and let this drop to its one commit. #144's content was reviewed by this pipeline at this exact head — see reports/eos-144-95692266.md; it is not re-reviewed here. |
| 4 | Low | PR body, line 1 | "Review 9569226..7a1d1e7 for this change" — 7a1d1e7 does not exist in this repository (git cat-file -t 7a1d1e7 → Not a valid object name). The head is 0a2a706. A reviewer following the instruction literally gets an error. The same defect is in eBoot#125 (cbc064b) and ebuild#144 (3708afb); none of the three end shas resolve in any of the three repos. (The "Stacked on #144" reference is correct — eos#144 is fix/repair-master-else-collision-and-guards at 9569226, the parent commit here — despite colliding numerically with the sibling ebuild PR.) |
Use 9569226..0a2a706, or drop the sha and say "the single commit at the head of this branch". |
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 — no #include, no link line, no target_link_libraries entry, no manifest dependency, so §5.1's direction-of-dependency law is untouched and the kernel boundary in §6 is unaffected by 0a2a706 itself. The one architectural tension is Finding 2: §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
- Correct the Problem section and the verification row: 25 of 26, not 26 of 26, and name eos#146 as the exception.
- 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. Behaviour-identical for open PRs in all 19 repos, and it removes the copy from each. - Keep the caller-side guard only if (2) is rejected — and then open the other 16.
- Fix the review range in the body to
9569226..0a2a706. - Merge #144 first; rebase this onto it so the PR is its one commit.
Not checked
- CI is pending, not failing. At bundle time (roughly 4 minutes after the PR opened)
Build & Test (Linux x86_64),Build & Test (Windows MSVC),Cross-compile ARM64 kernel,Default configuration (no tests, no product),QEMU Boot TestandAnalyze (C/C++)werependingwith 0s elapsed. I did not wait for them and I am not scoring them either way. Everything that had finished was green, includingHost Tests (x86_64),Cross-compile ARM Cortex-M4, all eleven Python matrix legs andpolicy / Policy / Linked Issue(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
0a2a706withyaml.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 eos, eBoot and ebuild; all three resulting files hash identically (fafeb8d17c7b). - The premise —
gh run list --workflow "Linked issue policy" --status failure, then every head branch resolved to its PR and its state read. Result in Finding 1: #102, #112, #113, #125 CLOSED; #75, #77, #79, #80, #104, #105, #106, #108, #109, #110, #111, #114, #115, #116, #117, #118, #119, #122, #129, #134, #135 MERGED; #146 OPEN. - 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 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. - No build or test was run against this PR's tree locally. The eos clone is checked out elsewhere and was not moved; everything above was read from
origin/masterand a fetched read-only ref. - #144's content is not re-reviewed here, per the brief's rule against repeating a point already made; see
reports/eos-144-95692266.md.
Automated architecture review of 0a2a7067d126 — 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.
Contributor
Author
|
Finding 1 — 25 of 26, not 26 of 26: you are right, and my own classification script had printed the exception and I still wrote "every one". Re-resolved tonight, branch by branch: 21 MERGED, 4 CLOSED, 1 OPEN — the open one is eos#146, where the policy is correctly failing and this guard does not touch it. Body corrected with the correction left visible. 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. Body edits only; nothing pushed to this branch. |
Contributor
Author
|
Finding 3 — now stated in the body's first paragraph in the merger's terms rather than only as "land #144 first": merging this as-is lands #144's diff (10 files, +142/−31, including Findings 1, 2 and 4 were answered above (25 of 26; .github#9 with the structural test — #10 was a duplicate and is closed in its favour; |
This was referenced Sep 17, 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.
Closes #153
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 against the policy and fails it.On 2026-09-14 that produced 26 failing
Linked issue policyruns in this repository: 25 on merged or closed branches, triggered by body edits on already-merged PRs — and one, on eos#146 (open), where the policy is correctly failing and this change does not touch it. (An earlier version of this paragraph said all 26 were closed; the review caught it, and the resolution below shows the 26th.) 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. Only an open PR is a decision still to be made.What it does not change: the open PRs in the org that currently fail this policy (other authors, in
eosandebuild) 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
Commands and their output rather than a table of verdicts:
Upstream: the caller is byte-identical in 19 repositories; the guard is moved into the reusable job in embeddedos-org/.github#9. Once that lands and this caller's pin is bumped, this guard is redundant — at that point close this PR rather than merge it; until then it is the only guard this repository has. (A second upstream PR, .github#10, duplicated #9 and is being closed in its favour.)