fix(journal): git_sha records a clean revision for a dirty working tree, contradicting its own contract
まだ誰も着手していません。
評価
調査の方向性
internal/onebox/service.go:159 から始め、service.go:139 と cmd/ob/commands.go:680 にあるその呼び出し元をたどってください。internal/compose/payload.go:33 が作業ツリーのファイルをどのように取得し、internal/engine/audit.go:37 が GIT の値をどのように表示するかを確認してください。作業完了の条件は、作業ツリーの dirty な payload の変更と未追跡の payload の変更が、単なるクリーンな HEAD SHA として報告されないこと、そして git ではないディレクトリでは現在の動作が維持されることです。提案されている表現のうち、maintainers がどれを望んでいるかを確認してください。
索引モデルが issue の本文から書いたものです。
説明
Summary
gitShortSHA documents that it omits a dirty revision. It does not. Every operation records the bare HEAD SHA regardless of working-tree state, so ob audit attests a commit for a release whose payload may not match that commit.
The contract, and the code
internal/onebox/service.go:159:
// gitShortSHA is the working tree's revision, recorded on every operation so a
// journal entry can be traced to the code that produced it. An unavailable or
// dirty revision is simply absent rather than guessed at.
func gitShortSHA(ctx context.Context, dir string) string {
out, err := exec.CommandContext(ctx, "git", "-C", dir, "rev-parse", "--short=7", "HEAD").Output()
if err != nil {
return ""
}
return strings.TrimSpace(string(out))
}
The comment promises three behaviors. Two hold: unavailable git returns "", and nothing is guessed. The third — that a dirty revision is absent — is not implemented anywhere. There is no working-tree check in this function or its callers (service.go:139, cmd/ob/commands.go:680).
git rev-parse --short=7 HEAD succeeds on a dirty tree. Verified on a scratch repository:
$ echo TAMPERED >> a.txt
$ git status --porcelain
M a.txt
$ git rev-parse --short=7 HEAD
20da80a
$ echo $?
0
So a dirty tree records a clean SHA. Either the comment or the code is wrong; both cannot stand.
Why it matters
This is not cosmetic, because the payload is copied from the working tree, not from git. StagePayloadContext (internal/compose/payload.go:33) walks bind sources and env files and copies whatever is on disk — including uncommitted modifications and untracked files. The recorded SHA and the shipped bytes are independently determined and can disagree.
The consequence is operator-visible. Engine.Audit (internal/engine/audit.go:37) prints a GIT column per release:
RELEASE ACTION OPERATOR GIT OUTCOME STARTED
20260902-054001-7e6ad74… deploy … 7e6ad74 ok …
An operator reading that row concludes the release contains 7e6ad74. If someone deployed with edits in the tree, it does not, and nothing in the journal, plan, or audit output says so. That is the failure mode docs/product.md is written against — evidence that reads as proof of something it never checked. The product statement promises "evidence-backed execution"; a provenance field that silently rounds "HEAD plus edits" to "HEAD" is the one place that should not round.
Worth being precise about the threat model: product.md already concedes that "a compromised host can lie about its own evidence." This is different and, I think, worse — an uncompromised host recording a misleading fact by construction, with no adversary involved.
Precedent in this codebase
Onebox already treats dirty-state as material, for its own binary:
internal/buildinfo/info.go:25carriesDirty.cmd/ob/version.go:72rendersrevision+dirty.cmd/ob/doctor.go:218raises a warning: "the runner was built from a dirty checkout".internal/engine/status.go:83appends+dirtyto the runner line.
So the standard is established and applied to the runner, while the payload — the thing actually being shipped to production — is exempt. That asymmetry is the argument; nothing here is imported from outside the project's own values.
Proposal
Detect the working-tree state where the SHA is captured, and stop at recording it — no refusal, no policy. Three shapes, in my order of preference:
- Suffix, matching
version.go. Record7e6ad74+dirty. Consistent with how the runner already reports itself, needs no schema change, and survives into the audit table's existingGITcolumn. - Separate field. Add
payload_dirtynext togit_shain the journal record and plan artifact, and let each surface render it. Cleaner for machine consumers, at the cost of a field in the record schema. - Honor the comment literally — omit the SHA when dirty. I would not: "based on 7e6ad74 plus edits" is strictly more useful to an operator than "-", and a bare dash is indistinguishable from "not a git repository".
Untracked files should count as dirty. git status --porcelain reports them, and they are copied into the release by the payload walk, so excluding them would reintroduce the same gap in a smaller form.
A follow-on, if 1 or 2 lands: ob doctor could warn on a dirty payload the way it already warns on a dirty runner. That would be a policy addition rather than a correctness fix, so it belongs behind this, not inside it.
Notes
- Reproduced against
8500ab7. - Cost is one
git status --porcelainper operation on the config directory, alongside therev-parsealready being run. - Non-git directories are unaffected:
rev-parsefails,""is recorded, and no claim is made — that path is already correct.
- 主要言語
- Go
- スター
- 3
- フォーク
- 0
- 平均マージ
- 2時間 44分
- マージ済み PR(30日)
- 49
コントリビューションガイド
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
labstack/onebox のほかの issue
-
bug
難易度 1/5 1時間未満 初心者へのやさしさ 80/100
-
bug
難易度 1/5 1時間未満 初心者へのやさしさ 85/100
-
bug
難易度 1/5 1時間未満 初心者へのやさしさ 85/100
-
bug
難易度 3/5 1〜2日 初心者へのやさしさ 65/100
-
難易度 4/5 3〜5日 初心者へのやさしさ 55/100
labstack/onebox の issue をすべて見る
似ている issue
-
ai-generated
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
-
難易度 2/5 1〜3時間 初心者へのやさしさ 72/100
SiaFoundation/indexd#1116 ·
-
難易度 2/5 1〜3時間 初心者へのやさしさ 84/100
-
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
-
triage
難易度 2/5 1〜3時間 初心者へのやさしさ 82/100
wakatime/wakatime-cli#1585 ·