Skip to content

fix: spec-author secrets 声明 + spec-check 精化(v1.5.3,W0-C4 #133,ADR-0050) - #39

Merged
randypanding merged 1 commit into
mainfrom
fix-spec-author-round2
Aug 21, 2026
Merged

randypanding merged 1 commit into
mainfrom
fix-spec-author-round2

Conversation

@randypanding

Copy link
Copy Markdown
Contributor

两个实测问题(首跑 run 32452510063):\n1. workflow_call 未声明 secrets——conductor 传 secrets 时 workflow 级 startup_failure(调用方向未声明的 secret 传值非法)。声明 LLM_API_KEY/CB_APP_ID/AGENT_APP_SECRET(environment 类型,required)。\n2. spec-check 两处精化:(a) 模型输出剥 ```围栏/前导说明行(首跑实测被围栏包裹→误判缺 frontmatter);(b) 注入豁免改同行引号段+16字否定前缀+『即fail』后缀——正例 IR-0001 v3 过、4 条注入负例全拦(回归套件在 PR 描述外的本地跑里)。

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@randypanding, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Limit details: You’ve used all 10 included reviews currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 11bfff9e-8a55-46bb-8d60-4b7be14b4fe1

📥 Commits

Reviewing files that changed from the base of the PR and between 5d4e0f1 and 9aba050.

📒 Files selected for processing (2)
  • .github/workflows/spec-author.yml
  • scripts/spec-check.py

Comment @coderabbitai help to get the list of available commands.

@randypanding
randypanding merged commit d36123f into main Aug 21, 2026
12 checks passed
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix spec-author secrets declaration and harden spec-check parsing/injection scan

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

Grey Divider

AI Description

• Declare required workflow_call secrets to prevent startup_failure on conductor invocations
• Normalize model outputs (strip code fences/leading text) before frontmatter validation
• Tighten injection-scan exemptions to reduce false positives/false negatives
Diagram

graph TD
  conductor{{"conductor"}} --> wf(["spec-author workflow"]) --> check["spec-check.py"] --> out["spec draft / PR"]
  secrets[("Environment secrets")] --> wf
  wf --> llm{{"LLM provider"}}

  subgraph Legend
    direction LR
    _ext{{"External"}} ~~~ _wf(["Workflow"]) ~~~ _scr["Script"] ~~~ _sec[("Secret")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Enforce formatting purely via prompt/system rules
  • ➕ Keeps validation logic simpler (less heuristic cleanup)
  • ➕ Shifts responsibility to model instruction compliance
  • ➖ Empirically brittle: model can still wrap outputs in fences or add preface text
  • ➖ Reduces defense-in-depth for automated pipelines
2. Parse markdown to locate first frontmatter block (AST-based)
  • ➕ More robust than regex stripping for edge cases
  • ➕ Can precisely identify frontmatter regardless of surrounding text
  • ➖ Adds dependency/complexity for a transitional g010 script
  • ➖ May be overkill for current scope and runner environment
3. Move injection exemptions to structured, frontmatter-aware scanning
  • ➕ Avoids exempting matches due to quoted YAML values or unrelated sections
  • ➕ Better separation between metadata and body semantics
  • ➖ Requires defining/specifying what constitutes 'body semantics' more formally
  • ➖ More refactor than necessary for the current targeted false-positive issues

Recommendation: Keep this PR’s defensive, validator-side hardening: declaring workflow_call secrets is required for correctness, and pre-stripping fences/leading lines plus tightening exemption rules improves reliability without expanding dependencies. Consider an AST-based frontmatter locator later if spec-check grows beyond g010.

Files changed (2) +42 / -10

Bug fix (1) +36 / -10
spec-check.pyHarden spec-check preprocessing and injection-scan exemptions +36/-10

Harden spec-check preprocessing and injection-scan exemptions

• Normalizes model output by stripping leading/trailing markdown fences and discarding preface lines before the YAML frontmatter marker. Refines injection-scan exemptions to use same-line quote bracketing, expands negation window, and adds a 'guard suffix' exemption for defensive statements like '...即 fail', reducing misclassification.

scripts/spec-check.py

Other (1) +6 / -0
spec-author.ymlDeclare required workflow_call environment secrets +6/-0

Declare required workflow_call environment secrets

• Adds explicit workflow_call secrets declarations (LLM_API_KEY, CB_APP_ID, AGENT_APP_SECRET) as required environment-type secrets. Prevents startup_failure when conductor passes secrets and aligns with policies that disallow secrets-inherit.

.github/workflows/spec-author.yml

@randypanding
randypanding deleted the fix-spec-author-round2 branch August 21, 2026 06:05
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Invalid secrets schema 🐞 Bug ≡ Correctness
Description
.github/workflows/spec-author.ymlon.workflow_call.secrets 下为 secrets 声明了 `{ type: environment
},该字段不在 GitHub Actions 语法中,可能导致 workflow 解析/校验失败。并且官方文档明确提示 workflow_call 不支持 environment`
语义,试图以此传递/区分 environment secrets 也不会按预期工作。
Code

.github/workflows/spec-author.yml[R17-19]

+      LLM_API_KEY:      { type: environment, required: true }
+      CB_APP_ID:        { type: environment, required: true }
+      AGENT_APP_SECRET: { type: environment, required: true }
Relevance

●●● Strong

PR #39 directly addresses workflow_call secret declarations; startup failure from undeclared secrets
confirms relevance.

PR-#39

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR 新增的 secrets 声明包含 type: environment。GitHub 官方 workflow 语法对 on.workflow_call.secrets
的示例与字段说明仅包含 required/description,且文档明确警告 workflow_call 不支持 environment secrets 传递。

.github/workflows/spec-author.yml[14-19]
🌐 on.workflow_call.secrets defines a map of secrets; per-secret it documents identifiers and required boolean (and examples use only required/description, not type).
🌐 Warns that environment secrets cannot be passed from the caller workflow because on.workflow_call does not support the environment keyword, and including it uses the environment secret instead of caller-passed secret.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Reusable workflow secrets are defined with `{ type: environment, required: true }`, but `on.workflow_call.secrets.<secret_id>` only supports fields like `required` (and optionally `description`). Using an unsupported key can fail workflow validation and will not implement “environment-typed secrets”.

### Issue Context
GitHub docs: `on.workflow_call.secrets` is a map of secret identifiers with `required` boolean; additionally `workflow_call` does not support environment secrets being passed from the caller.

### Fix Focus Areas
- .github/workflows/spec-author.yml[14-19]

### Proposed fix
- Replace each secret entry with:
 - `LLM_API_KEY: { required: true }`
 - `CB_APP_ID: { required: true }`
 - `AGENT_APP_SECRET: { required: true }`
- Optionally add `description:` fields.
- If you truly need environment secrets behavior, don’t try to encode it in `workflow_call.secrets`; instead use job-level `environment:` in the called workflow and rely on that environment’s secrets (not caller-passed secrets).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Quote exemption too broad 🐞 Bug ⛨ Security
Description
scripts/spec-check.py 的注入扫描新增“同行引号段豁免”逻辑仅判断同一行命中段前后是否分别出现过引号字符,这会把不在引号包裹内的命中段误判为
quoted,从而跳过注入报错,造成注入条款漏检。攻击者/误写 spec 只需在同一行前后放置无关引号即可触发豁免。
Code

scripts/spec-check.py[R126-129]

+            prefix = text[line_start:m.start()]
+            suffix = text[m.end():line_end]
+            quoted = (('"' in prefix and '"' in suffix)
+                      or (QL in prefix and QR in suffix))
Relevance

●●● Strong

PR #39 explicitly tightened quote exemptions; a same-line window can still incorrectly suppress
injection findings.

PR-#39

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
新逻辑把 quoted 判定为“prefix 含引号且 suffix 含引号”,并未验证命中段处于同一对引号之内;因此任何同一行出现一前一后的引号都会让命中段被豁免。该判断直接控制
continue 跳过报错。

scripts/spec-check.py[111-141]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new injection-scan exemption treats a match as “quoted” if there is any `"` before the match and any `"` after it on the same line. This does not prove the match is inside a quoted span, and can wrongly exempt real injection content.

### Issue Context
This script is a gatekeeper (“模式命中即报,宁枉勿纵”). Exemptions must be precise; otherwise a malicious/accidental spec line can bypass injection detection.

### Fix Focus Areas
- scripts/spec-check.py[121-137]

### Proposed fix
Implement proper ‘inside quoted span’ detection per-line:
- Extract the full line `[line_start:line_end]`.
- Find all quoted spans on that line via regex (support at least: `"..."`, `“...”`, optionally `'...'`).
- Consider the match exempt **only if** `m.start()`/`m.end()` is fully within one quoted span.
- Avoid relying on mere presence of quotes in prefix/suffix.

Add unit-test-like fixtures (even if just a small self-test block) for:
- injection inside quotes (should exempt)
- injection outside quotes but line contains other quotes (must NOT exempt)
- nested/multiple quoted segments
- single quotes if you want to keep doc-comment promise of supporting them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Unused DQ variable 🐞 Bug ⚙ Maintainability
Description
scripts/spec-check.py 新增的 DQ = '"' 未被使用,会误导读者以为存在更完整的引号类型处理逻辑。保留无用变量会增加后续维护成本。
Code

scripts/spec-check.py[R116-118]

+    QL = '\u201c'
+    QR = '\u201d'
+    DQ = '"'
Relevance

●●● Strong

Unused DQ is a trivial local clarity issue; removing the dead variable is likely accepted.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
文件中定义了 DQ,但 quoted 判定使用的是字面量 ",未引用该变量。

scripts/spec-check.py[116-130]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DQ = '"'` is defined but never used.

### Issue Context
It sits next to QL/QR and looks like part of quote-handling logic, but currently has no effect.

### Fix Focus Areas
- scripts/spec-check.py[116-119]

### Proposed fix
- Remove `DQ` if not needed, or actually use it in the quoted-span logic (ideally after tightening that logic as well).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +5 more
Review mode: ⚖️ Balanced: 改动涉及 GitHub Actions secret 合约及 spec-check 的解析与注入检测逻辑,具有安全和行为回归风险,但编辑点有限,不属于需要多轮冗余审查的高密度变更。
ⓘ  2 issues published inline · 3 in summary

Grey Divider

Tip of the day
💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +17 to +19
LLM_API_KEY: { type: environment, required: true }
CB_APP_ID: { type: environment, required: true }
AGENT_APP_SECRET: { type: environment, required: true }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Invalid secrets schema 🐞 Bug ≡ Correctness

.github/workflows/spec-author.ymlon.workflow_call.secrets 下为 secrets 声明了 `{ type: environment
},该字段不在 GitHub Actions 语法中,可能导致 workflow 解析/校验失败。并且官方文档明确提示 workflow_call 不支持 environment`
语义,试图以此传递/区分 environment secrets 也不会按预期工作。
Agent Prompt
### Issue description
Reusable workflow secrets are defined with `{ type: environment, required: true }`, but `on.workflow_call.secrets.<secret_id>` only supports fields like `required` (and optionally `description`). Using an unsupported key can fail workflow validation and will not implement “environment-typed secrets”.

### Issue Context
GitHub docs: `on.workflow_call.secrets` is a map of secret identifiers with `required` boolean; additionally `workflow_call` does not support environment secrets being passed from the caller.

### Fix Focus Areas
- .github/workflows/spec-author.yml[14-19]

### Proposed fix
- Replace each secret entry with:
  - `LLM_API_KEY: { required: true }`
  - `CB_APP_ID: { required: true }`
  - `AGENT_APP_SECRET: { required: true }`
- Optionally add `description:` fields.
- If you truly need environment secrets behavior, don’t try to encode it in `workflow_call.secrets`; instead use job-level `environment:` in the called workflow and rely on that environment’s secrets (not caller-passed secrets).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread scripts/spec-check.py
Comment on lines +126 to +129
prefix = text[line_start:m.start()]
suffix = text[m.end():line_end]
quoted = (('"' in prefix and '"' in suffix)
or (QL in prefix and QR in suffix))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Quote exemption too broad 🐞 Bug ⛨ Security

scripts/spec-check.py 的注入扫描新增“同行引号段豁免”逻辑仅判断同一行命中段前后是否分别出现过引号字符,这会把不在引号包裹内的命中段误判为
quoted,从而跳过注入报错,造成注入条款漏检。攻击者/误写 spec 只需在同一行前后放置无关引号即可触发豁免。
Agent Prompt
### Issue description
The new injection-scan exemption treats a match as “quoted” if there is any `"` before the match and any `"` after it on the same line. This does not prove the match is inside a quoted span, and can wrongly exempt real injection content.

### Issue Context
This script is a gatekeeper (“模式命中即报,宁枉勿纵”). Exemptions must be precise; otherwise a malicious/accidental spec line can bypass injection detection.

### Fix Focus Areas
- scripts/spec-check.py[121-137]

### Proposed fix
Implement proper ‘inside quoted span’ detection per-line:
- Extract the full line `[line_start:line_end]`.
- Find all quoted spans on that line via regex (support at least: `"..."`, `“...”`, optionally `'...'`).
- Consider the match exempt **only if** `m.start()`/`m.end()` is fully within one quoted span.
- Avoid relying on mere presence of quotes in prefix/suffix.

Add unit-test-like fixtures (even if just a small self-test block) for:
- injection inside quotes (should exempt)
- injection outside quotes but line contains other quotes (must NOT exempt)
- nested/multiple quoted segments
- single quotes if you want to keep doc-comment promise of supporting them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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