Dev - #2720
Closed
namastex888 wants to merge 15 commits into
Closed
Dev#2720namastex888 wants to merge 15 commits into
namastex888 wants to merge 15 commits into
Conversation
…channel default (#2674, #2675) admit now asks whether v<version> already carries a published GitHub Release and refuses the run when it does. A published release is immutable and this pipeline can no longer replay one: reconcile-release-assets.sh expects the current 20/28/36-asset delivery fanout while every pre-fanout release carries 12 that can never be extended, and reconcile-channel-manifests.sh derives a deterministic midnight-UTC released_at that no longer cmp-matches the wall-clock timestamps already on main. Both failures land AFTER the release is published and locked, so the rule "always allocate a fresh version" is now enforced mechanically instead of remembered. Drafts stay admissible so an interrupted run can finish the release it opened, and an ambiguous API answer fails closed. admit gains contents:read (never write) for the query. publish loses its dead CHANNEL="${INPUT_CHANNEL:-}" -> stable default. admit already exits 1 on an empty/unknown channel and both callers always pass one; had it ever fired, publish would have uploaded a 36-asset stable inventory that finalize aborts on, leaving a published-but-unfinalized release. publish now reads inputs.channel raw, exactly like finalize and manifests, and the stale "workflow_run defaults to stable" comment is gone. Tests: the reconcile-release-assets fake now returns TWO attestations per published tarball (GitHub's in-toto immutable-release attestation first, ours second), pinning that production selects by predicate type rather than by position or count. release-docs pins the new admit guard and the channel wiring. Refs #2674, #2675
…2669) The scanner only matched bare `uses:` scalars and accepted any 40-hex run inside the value, so a quoted pin was skipped entirely and `@<sha>oops` passed as a valid pin. - extract_pin() recognises bare, 'single-quoted' and "double-quoted" scalars - the ref must span the WHOLE scalar and end in exactly 40 hex chars - scan set now also covers action manifests tracked outside .github/ (dedupe makes the overlap harmless) - offline `--extract` mode exposes the matcher for tests (no gh, no network) Closes #2669
…ts (#2705) The shared-workspace git-state freeze in AGENTS.md — shared-workspace subagents never run checkout/switch/reset/stash/rebase, only the orchestrator moves HEAD — was enforced by brief prose alone. #2705 asked whether a dispatch-level guard could enforce it without false-positives. It can, on Claude Code. Measured on 2.1.220: the PreToolUse payload carries agent_id/agent_type. Main-thread Bash calls arrive with agent_id: null, agent_type: null; a spawned subagent's arrive with agent_id: "<id>", agent_type: "general-purpose" under the same session_id and the same cwd. That is the orchestrator-vs-subagent discriminator the freeze needs, and nothing else in the payload provides it. The new PreToolUse:Bash handler (priority 2, alongside the other deny-guards) walks a compound command left to right, tracks literal cd/pushd, honours git -C, and denies a frozen subcommand only when `git rev-parse --show-toplevel` of the target directory equals that of the session cwd. A linked worktree resolves to a different top level, so `git worktree add <path>` followed by `git -C <path> switch` — the documented escape hatch — is untouched. Every ambiguity fails OPEN, deliberately: no agent_id (main thread, Codex, a client that drops the field), a non-literal cd/-C target, --git-dir/--work-tree overrides, or an unresolvable working tree all allow. Unlike branch-guard, the freeze has no server-side backstop, so a wrong deny has no escape hatch; the founding incident was an accidental HEAD move, and a guard that catches the literal forms and never fires on a legitimate one beats a guard nobody keeps enabled. It is a guardrail, not a sandbox. Also lifts branch-guard's quote masker into src/hooks/shell-quoting.ts so both Bash classifiers share one implementation. Coverage gap kept explicit: Codex PreToolUse carries no subagent identity, and `genie launch` Warp panes have no hook surface at all (they are worktree-isolated by construction, so exempt). Full assessment per runtime is recorded on #2705.
Two regressions in the #2669 matcher, both from matching the whole line: - a quoted pin inside a trailing comment shadowed the real bare pin, because the unanchored uses: patterns re-matched at the comment; - a flow-mapping step (`- { uses: owner/repo@<sha>, name: X }`) was silently skipped, because the comma stayed glued to the bare scalar and the anchored pin_re then rejected it. Cut to the first `uses:` key, ltrim, and match the three scalar spellings anchored on that remainder; bare scalars now also terminate at `,` and `}`. Covered by two regression tests that fail on the pre-fix script. bash 3.2 compatible; the live run still reports the same 10 ok pins.
…alars fix(ci): check-action-pins matches quoted uses scalars, anchors SHA (#2669)
…st git call Two verifier defects in the freeze guard, one in each direction. A subshell was a false positive. `(cd /other && git switch main)` denied, because the token `(cd` is not `cd`, so the directory change was dropped and the git call was judged against the session cwd — a deny against a command that never touches the shared checkout, in a module whose whole posture is fail-open. Statements now carry their subshell parentheses: a leading `(` opens a directory scope and a trailing `)` closes it, so the `cd` is tracked inside the group and does not leak past it. Both spellings parse, `(cd /x` and `( cd /x`. That accounting only holds if the parentheses counted are the shell's grouping and nothing else, so command and process substitutions are masked first — the stray `)` of a `$(pwd)` would otherwise close a real group early and leak its `cd` outward. They mask to `$`, which the non-literal-path test already rejects, keeping `git -C $(pwd)` failing open as before. The cost is that a frozen call nested inside a substitution is no longer seen; that gap is documented in the module header and pinned by a test. A compound command was a false negative. `git -C /other switch main && git switch dev` was allowed, because the finder returned the first frozen invocation and the guard, finding it outside the shared checkout, stopped — a legitimate lead call shielded a frozen one behind it. Every frozen invocation is now collected and resolved, and the deny is raised for the first one that lands in the shared root. The "no git subprocess unless a frozen subcommand is present" property is preserved, and repeated directories resolve once. 15 regression tests covering both directions; 60 existing tests unchanged.
feat(hooks): mechanical git-freeze guard for shared-workspace subagents (#2705)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR bumps plugin versions to ChangesRelease channel retirement
GitHub Action pin validation
Subagent git-state freeze
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73007d1f4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else tokens[0] = head; | ||
| } | ||
| let closed = 0; | ||
| while (tokens.length > 0 && tokens[tokens.length - 1].endsWith(')')) { |
There was a problem hiding this comment.
When a grouped command redirects output, such as (cd /other && git status) > /tmp/out && git switch dev, the closing parenthesis is attached to status rather than the final out token, so this loop records no closure and leaves /other as the active scope. The later git switch actually runs from the original shared workspace, but the guard resolves it against /other and allows the prohibited HEAD mutation; detect closing parentheses before trailing redirections.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| if (!/\bgit\b/.test(masked)) return []; | ||
| const scopes: (string | null)[] = [cwd]; | ||
| const found: GitInvocation[] = []; | ||
| for (const segment of masked.split(SEGMENT_SEPARATORS)) { |
There was a problem hiding this comment.
When a subagent uses a heredoc to write documentation or a script containing text such as git switch dev, splitting on newlines treats that body line as an executable statement and denies the entire Bash call even though the shell only writes the text. This makes common commands such as cat <<'EOF' ... EOF unusable whenever their payload discusses a frozen command; mask or skip heredoc bodies before classifying statements.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/handlers/git-freeze-guard.ts`:
- Around line 351-358: Seed rootCache with the existing sharedRoot result for
sessionCwd before defining or using rootOf, so rootOf(sessionCwd) reuses the
initial resolveWorktreeRoot call. Preserve the null early return and cached
resolution behavior for other directories.
In `@src/hooks/shell-quoting.ts`:
- Around line 1-67: Add a colocated shell-quoting.test.ts suite for
maskQuotedRegions, covering unquoted text, single- and double-quoted regions,
escaped characters in double quotes, unterminated quotes, and preservation of
output length. Keep the tests focused on the shared utility and include the
backslash-escape edge case directly.
- Around line 33-38: Update stepUnquoted to recognize an unquoted backslash
before quote delimiters, consuming the escape and keeping the parser in state
'none' so escaped quotes remain unmasked literal command text. Preserve the
existing quote-region transitions for unescaped single and double quotes and
normal character passthrough.
In `@tests/integration/check-action-pins-matcher.test.ts`:
- Around line 107-115: Update the line filter in the “every SHA-pinned uses”
test to match the same anchored format as extract_pin: require the 40-character
hexadecimal SHA to terminate the value, allowing only whitespace and an optional
trailing comment afterward. Keep the existing workflow-file selection and
extraction assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03f9b95b-68f4-4a20-b3e8-718a588df9e5
📒 Files selected for processing (16)
.claude-plugin/marketplace.json.github/workflows/release-publish.ymlpackage.jsonplugins/genie/.claude-plugin/plugin.jsonplugins/genie/.codex-plugin/plugin.jsonplugins/genie/package.jsonplugins/hermes-genie/plugin.yamlscripts/check-action-pins.shscripts/reconcile-release-assets.test.tsscripts/release-docs.test.tssrc/hooks/__tests__/git-freeze-guard.test.tssrc/hooks/handlers/branch-guard.tssrc/hooks/handlers/git-freeze-guard.tssrc/hooks/index.tssrc/hooks/shell-quoting.tstests/integration/check-action-pins-matcher.test.ts
Comment on lines
+351
to
+358
| const sharedRoot = deps.resolveWorktreeRoot(sessionCwd); | ||
| if (sharedRoot === null) return; | ||
|
|
||
| const rootCache = new Map<string, string | null>(); | ||
| const rootOf = (dir: string): string | null => { | ||
| if (!rootCache.has(dir)) rootCache.set(dir, deps.resolveWorktreeRoot(dir)); | ||
| return rootCache.get(dir) ?? null; | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Redundant resolveWorktreeRoot call on the common (no-cd) path.
sharedRoot is resolved directly for sessionCwd (line 351) before rootCache exists. When a frozen invocation's dir equals sessionCwd (i.e., no cd/-C redirection — the typical single-command case), rootOf(target.dir) at line 361 misses the cache and re-invokes resolveWorktreeRoot, spawning a second git rev-parse subprocess for the same directory already resolved on line 351.
♻️ Proposed fix — seed the cache with the already-resolved root
const sharedRoot = deps.resolveWorktreeRoot(sessionCwd);
if (sharedRoot === null) return;
- const rootCache = new Map<string, string | null>();
+ const rootCache = new Map<string, string | null>([[sessionCwd, sharedRoot]]);
const rootOf = (dir: string): string | null => {
if (!rootCache.has(dir)) rootCache.set(dir, deps.resolveWorktreeRoot(dir));
return rootCache.get(dir) ?? null;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sharedRoot = deps.resolveWorktreeRoot(sessionCwd); | |
| if (sharedRoot === null) return; | |
| const rootCache = new Map<string, string | null>(); | |
| const rootOf = (dir: string): string | null => { | |
| if (!rootCache.has(dir)) rootCache.set(dir, deps.resolveWorktreeRoot(dir)); | |
| return rootCache.get(dir) ?? null; | |
| }; | |
| const sharedRoot = deps.resolveWorktreeRoot(sessionCwd); | |
| if (sharedRoot === null) return; | |
| const rootCache = new Map<string, string | null>([[sessionCwd, sharedRoot]]); | |
| const rootOf = (dir: string): string | null => { | |
| if (!rootCache.has(dir)) rootCache.set(dir, deps.resolveWorktreeRoot(dir)); | |
| return rootCache.get(dir) ?? null; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/handlers/git-freeze-guard.ts` around lines 351 - 358, Seed
rootCache with the existing sharedRoot result for sessionCwd before defining or
using rootOf, so rootOf(sessionCwd) reuses the initial resolveWorktreeRoot call.
Preserve the null early return and cached resolution behavior for other
directories.
Comment on lines
+1
to
+67
| /** | ||
| * Shell quote masking — shared by every PreToolUse:Bash classifier. | ||
| * | ||
| * Mask the interior of single/double-quoted shell regions with spaces, | ||
| * preserving string length so regex word-boundaries behave identically on the | ||
| * remaining unmasked characters. | ||
| * | ||
| * Closes a class of over-matches where a blocked substring (`gh pr merge N`, | ||
| * `git push origin main`, `git checkout main && git commit`) appearing inside | ||
| * a `--body` / `--message` / `-m` argument triggered a false-positive deny. | ||
| * | ||
| * Live reproducer: opening PR #1264 (the branch-guard subprocess-diagnostics | ||
| * fix) was blocked twice because the PR body described the very commands the | ||
| * hook denies. Required a workaround — paraphrase every literal occurrence — | ||
| * that doesn't generalize. | ||
| * | ||
| * Scope: handles single-quotes (no escapes), double-quotes with `\X` escapes, | ||
| * and unterminated quotes (mask to end of string — safer than the alternative | ||
| * of leaving a runaway region unmasked). Backtick command substitution and | ||
| * heredocs are intentionally not parsed — they're rare in agent-issued | ||
| * commands and falling back to fully unmasked treatment is fail-closed for | ||
| * the original policy, which matches the hook's overall posture. | ||
| */ | ||
|
|
||
| type QuoteState = 'none' | 'single' | 'double'; | ||
|
|
||
| interface MaskStep { | ||
| out: string; | ||
| next: QuoteState; | ||
| consumed: number; | ||
| } | ||
|
|
||
| /** Unquoted char: pass through, or open a quote region. */ | ||
| function stepUnquoted(ch: string): MaskStep { | ||
| if (ch === "'") return { out: ' ', next: 'single', consumed: 1 }; | ||
| if (ch === '"') return { out: ' ', next: 'double', consumed: 1 }; | ||
| return { out: ch, next: 'none', consumed: 1 }; | ||
| } | ||
|
|
||
| /** Single-quoted char: always masked; `'` closes the region (no escapes in bash single-quotes). */ | ||
| function stepSingleQuoted(ch: string): MaskStep { | ||
| return { out: ' ', next: ch === "'" ? 'none' : 'single', consumed: 1 }; | ||
| } | ||
|
|
||
| /** Double-quoted char: always masked; `\X` consumes two chars; `"` closes. */ | ||
| function stepDoubleQuoted(ch: string, hasNext: boolean): MaskStep { | ||
| if (ch === '\\' && hasNext) return { out: ' ', next: 'double', consumed: 2 }; | ||
| return { out: ' ', next: ch === '"' ? 'none' : 'double', consumed: 1 }; | ||
| } | ||
|
|
||
| export function maskQuotedRegions(cmd: string): string { | ||
| let out = ''; | ||
| let state: QuoteState = 'none'; | ||
| let i = 0; | ||
| while (i < cmd.length) { | ||
| const step: MaskStep = | ||
| state === 'double' | ||
| ? stepDoubleQuoted(cmd[i], i + 1 < cmd.length) | ||
| : state === 'single' | ||
| ? stepSingleQuoted(cmd[i]) | ||
| : stepUnquoted(cmd[i]); | ||
| out += step.out; | ||
| state = step.next; | ||
| i += step.consumed; | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No colocated test for this shared, security-sensitive masking utility.
shell-quoting.ts is now consumed by two guards (one fail-closed, one fail-open) but ships without its own shell-quoting.test.ts. As per path instructions, src/**/*.ts should "colocate tests as *.test.ts". A dedicated suite would exercise masking edge cases (like the backslash-escape gap above) directly, rather than relying on incidental coverage through consumers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/shell-quoting.ts` around lines 1 - 67, Add a colocated
shell-quoting.test.ts suite for maskQuotedRegions, covering unquoted text,
single- and double-quoted regions, escaped characters in double quotes,
unterminated quotes, and preservation of output length. Keep the tests focused
on the shared utility and include the backslash-escape edge case directly.
Source: Path instructions
Comment on lines
+33
to
+38
| /** Unquoted char: pass through, or open a quote region. */ | ||
| function stepUnquoted(ch: string): MaskStep { | ||
| if (ch === "'") return { out: ' ', next: 'single', consumed: 1 }; | ||
| if (ch === '"') return { out: ' ', next: 'double', consumed: 1 }; | ||
| return { out: ch, next: 'none', consumed: 1 }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'src/hooks/shell-quoting.ts' || true
echo "== outline =="
ast-grep outline src/hooks/shell-quoting.ts || true
echo "== relevant source =="
cat -n src/hooks/shell-quoting.ts
echo "== consumers/usages =="
rg -n "maskQuotedRegions|shell-quoting|mask.*quote|quoted" src/hooks src -g '*.ts' | head -n 200
echo "== related tests =="
rg -n "shell-quoting|quote|mask" src/test src -g '*.test.ts' -g '*.ts' || trueRepository: automagik-dev/genie
Length of output: 17323
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
def maskQuotedRegions(cmd):
def stepSingleQuoted(ch):
if ch == "'":
return {'out': ' ', next='none', 'consumed': 1}
return {'out': ' ', next='single', 'consumed': 1}
def stepDoubleQuoted(ch, has_more):
if ch == '"':
return {'out': ' ', next='none', 'consumed': 1}
if ch == '$' and has_more and cmd[i + 1] in "'\"":
return {'out': ' ', next='double', 'consumed': 2}
return {'out': ' ', next='double', 'consumed': 1}
def stepUnquoted(ch):
if ch == "'":
return {'out': ' ', next='single', 'consumed': 1}
if ch == '"':
return {'out': ' ', next='double', 'consumed': 1}
return {'out': ch, next='none', 'consumed': 1}
i = 0
out = []
state = 'none'
while i < len(cmd):
if state in ("single", "double"):
ch = cmd[i]
step = stepDoubleQuoted(ch, i + 1 < len(cmd)) if state == "double" else stepSingleQuoted(ch)
else:
step = stepUnquoted(cmd[i])
i += 1
out.append(step["out"])
state = step["next"] if "next" in step else None
return ''.join(out)
print(maskQuotedRegions("git commit -m don\\'t\ touch main"))
print(maskQuotedRegions("git commit -m don' t\ touch main"))
print(maskQuotedRanges("echo \"\\'\""))
PYRepository: automagik-dev/genie
Length of output: 292
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
# Model the current mask implementation from shell-quoting.ts for read-only verification.
def maskQuotedRegions(cmd):
out = []
state = 'none'
i = 0
while i < len(cmd):
ch = cmd[i]
if state == 'single':
if ch == "'":
out.append(' ')
state = 'none'
else:
out.append(' ')
i += 1
continue
if state == 'double':
if ch == '"':
out.append(' ')
state = 'none'
else:
out.append(' ')
i += 1
continue
if ch == "'":
out.append(' ')
state = 'single'
i += 1
continue
if ch == '"':
out.append(' ')
state = 'double'
i += 1
continue
out.append(ch)
i += 1
return ''.join(out)
examples = [
"git commit -m don'\\t touch main", # original escaped representation if literal: git commit -m don\'t\ touch main
"git commit -m don' t touch main",
"echo \"'\"",
"echo \"\\\"\"",
]
for s in examples:
print(repr(s), "=>", repr(maskQuotedRegions(s)))
PY
echo "== bash behavior with /bin/bash if available =="
if [ -x /bin/bash ]; then
/bin/bash -n 'git commit -m don'\''t\ touch main' && echo 'parse_ok'
/bin/bash -c 'printf "[%s]\n" "don'\"'\"'t touch main"'
/bin/sh -c 'printf "[%s]\n" "don'\"'\"'t touch main"'
else
echo '/bin/bash unavailable'
fiRepository: automagik-dev/genie
Length of output: 480
Handle unquoted backslash before opening quote regions.
stepUnquoted opens quoted regions for every ' and ", but an unquoted backslash escapes the following character in bash. Escaped quotes like \'/\" should continue in state: 'none', leaving them unmasked as literal command text. As-is, the backslash is passed through and the next quote opens a masked region, which can hide real unquoted command content from branch/freeze guard pattern matching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/shell-quoting.ts` around lines 33 - 38, Update stepUnquoted to
recognize an unquoted backslash before quote delimiters, consuming the escape
and keeping the parser in state 'none' so escaped quotes remain unmasked literal
command text. Preserve the existing quote-region transitions for unescaped
single and double quotes and normal character passthrough.
Comment on lines
+107
to
+115
| test('every SHA-pinned uses: line in .github/workflows still extracts', () => { | ||
| const lines = readdirSync(workflowDir) | ||
| .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) | ||
| .flatMap((name) => readFileSync(join(workflowDir, name), 'utf8').split('\n')) | ||
| .filter((line) => /uses:/.test(line) && /@[0-9a-f]{40}/.test(line)); | ||
|
|
||
| expect(lines.length).toBeGreaterThan(0); | ||
| const pins = extract(...lines); | ||
| expect(pins.length).toBe(lines.length); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Loose filter regex can desync from the matcher's anchored pin logic.
/uses:/.test(line) && /@[0-9a-f]{40}/.test(line) matches any line containing uses: and a 40-hex run anywhere, but extract_pin requires the SHA to terminate the entire scalar. A future line with uses: plus an incidental 40-hex string elsewhere (e.g. mentioned in a trailing comment) would inflate the expected count without a matching extracted pin, failing this test for a reason unrelated to an actual regression.
🧪 Tighten the filter to require the SHA at the end of the value (before an optional comment)
const lines = readdirSync(workflowDir)
.filter((name) => name.endsWith('.yml') || name.endsWith('.yaml'))
.flatMap((name) => readFileSync(join(workflowDir, name), 'utf8').split('\n'))
- .filter((line) => /uses:/.test(line) && /@[0-9a-f]{40}/.test(line));
+ .filter((line) => /uses:\s*['"]?[^\s'"#]+@[0-9a-f]{40}['"]?\s*(#.*)?$/.test(line));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('every SHA-pinned uses: line in .github/workflows still extracts', () => { | |
| const lines = readdirSync(workflowDir) | |
| .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) | |
| .flatMap((name) => readFileSync(join(workflowDir, name), 'utf8').split('\n')) | |
| .filter((line) => /uses:/.test(line) && /@[0-9a-f]{40}/.test(line)); | |
| expect(lines.length).toBeGreaterThan(0); | |
| const pins = extract(...lines); | |
| expect(pins.length).toBe(lines.length); | |
| test('every SHA-pinned uses: line in .github/workflows still extracts', () => { | |
| const lines = readdirSync(workflowDir) | |
| .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) | |
| .flatMap((name) => readFileSync(join(workflowDir, name), 'utf8').split('\n')) | |
| .filter((line) => /uses:\s*['"]?[^\s'"#]+@[0-9a-f]{40}['"]?\s*(#.*)?$/.test(line)); | |
| expect(lines.length).toBeGreaterThan(0); | |
| const pins = extract(...lines); | |
| expect(pins.length).toBe(lines.length); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/check-action-pins-matcher.test.ts` around lines 107 - 115,
Update the line filter in the “every SHA-pinned uses” test to match the same
anchored format as extract_pin: require the 40-character hexadecimal SHA to
terminate the value, allowing only whitespace and an optional trailing comment
afterward. Keep the existing workflow-file selection and extraction assertions
unchanged.
The homolog branch never carried genie traffic — it stayed dormant from its
introduction (2026-05-12) onward — so the dev→homolog→stable middle tier was
pure cost: a third manifest, a third evidence fan-out, and a channel arm in
every guard, predicate, and admission table.
The channel set is now {stable, dev}. Stable stays main-only manual dispatch
with the two-maintainer production gate; dev automation is unchanged.
Producer side removed: the homolog workflow_run branch and its auto-dispatch
(release_ready=true) path, the homolog:homolog admission pair, the homolog
manifest target and its .well-known/homolog.json file, and the homolog arm of
every channel/source-branch validator.
Asset math: stable evidence fan-out drops from (stable homolog dev) to
(stable dev), so per platform stable is now tarball+bundle+intoto (3) plus
2 evidence channels x (descriptor+sigstore) (4) = 7, and across 4 platforms
28 assets / 8 delivery descriptors. dev is unchanged at 20 / 4.
Consumer side keeps two read-only back-compat aliases so an operator already
pinned to homolog is not broken. Both resolve to STABLE, not dev: homolog
ranked above dev in the retired ladder, so stable is the conservative landing
and silently subscribing those users to less-vetted dev builds would be a
safety regression they never consented to.
- updateChannel: 'homolog' stays in the Zod enum and transforms to 'latest'.
Dropping it outright would hard-fail the entire config parse — taking
every unrelated field down with it — for anyone who ever ran
`genie update --homolog`. The token never round-trips back to disk.
- GENIE_CHANNEL=homolog in install.sh warns and maps to stable.
The `--homolog` CLI flag is removed outright rather than aliased: an explicit
flag for a channel that no longer exists should fail loudly at the command
line, which is feedback the operator can act on. That asymmetry with the
config is deliberate — config is data at rest whose parse failure is silent
and total.
…t, README migration note
feat(release)!: remove the homolog channel (stable+dev only)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/support/codex-dogfood-harness.ts (1)
767-776: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
homologvalid in this legacy provenance reader.This validates the prior immutable release, not a new channel input. A prior stable release whose preserved automated provenance records
sourceBranch: "homolog"now fails dogfood before promotion, despite the workflow explicitly retaining historical signed bytes. Accepthomologhere only for legacy provenance and add a fixture for it.Proposed fix
- !/^(?:main|dev)$/.test(facts.sourceBranch) || + !/^(?:main|homolog|dev)$/.test(facts.sourceBranch) ||🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/support/codex-dogfood-harness.ts` around lines 767 - 776, Update the legacy provenance validation around sourceBranch in the verified previous-release reader to accept main, dev, and homolog, without broadening validation for new channel inputs. Add or update a fixture covering sourceBranch: "homolog" and ensure it passes the existing provenance checks.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/support/codex-dogfood-harness.ts`:
- Around line 767-776: Update the legacy provenance validation around
sourceBranch in the verified previous-release reader to accept main, dev, and
homolog, without broadening validation for new channel inputs. Add or update a
fixture covering sourceBranch: "homolog" and ensure it passes the existing
provenance checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5af8b38c-57ba-4b67-998a-a7b6941a93ef
⛔ Files ignored due to path filters (3)
AGENTS.mdis excluded by!*.mdREADME.mdis excluded by!*.mdSECURITY.mdis excluded by!*.md
📒 Files selected for processing (38)
.github/workflows/build-tarballs.yml.github/workflows/ci.yml.github/workflows/release-publish.yml.github/workflows/release.yml.github/workflows/sign-attest.yml.github/workflows/version.yml.well-known/homolog.jsoninstall.shplugins/genie/references/codex-integration-map.mdscripts/build-delivery-evidence.tsscripts/candidate-dogfood-matrix.test.tsscripts/candidate-dogfood-matrix.tsscripts/materialize-release-subjects.test.tsscripts/reconcile-channel-manifests.shscripts/reconcile-channel-manifests.test.tsscripts/reconcile-release-assets.shscripts/reconcile-release-assets.test.tsscripts/reconcile-release-note.shscripts/release-docs.test.tsscripts/release-generic-provenance.shscripts/release-generic-provenance.test.tsscripts/release-guard.shscripts/release-native-predicate.shscripts/validate-dogfood-matrix-evidence.test.tsscripts/validate-dogfood-matrix-evidence.tsscripts/validate-live-dogfood-evidence.tssrc/genie-commands/__tests__/update.test.tssrc/genie-commands/install.test.tssrc/genie-commands/install.tssrc/genie-commands/local-delivery-repair.test.tssrc/genie-commands/local-delivery-repair.tssrc/genie-commands/update.tssrc/genie.tssrc/lib/codex-activation.test.tssrc/lib/codex-delivery-evidence.tssrc/lib/codex-host-observation.test.tssrc/types/genie-config.tstests/support/codex-dogfood-harness.ts
💤 Files with no reviewable changes (3)
- .well-known/homolog.json
- src/genie-commands/local-delivery-repair.test.ts
- src/genie.ts
Contributor
|
Closing to consolidate: dev has advanced past this PR's snapshot (homolog removal #2721 + fixes landed; dev now at v5.260727.17), and per the maintainer's authoring rule the promotion must be automagik-genie-authored so the maintainer is the independent approver. Replacement promotion follows immediately, carrying everything. |
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
2 participants
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.
Summary by CodeRabbit
homologchannel across release, evidence, and update flows; channels are now limited tostableanddev.